We quote BOLT 2 on *local* above the *remote* checks (we quote it
again below when we do the local checks).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
1. l1 update_fee -> l2
2. l1 commitment_signed -> l2 (using new feerate)
3. l1 <- revoke_and_ack l2
4. l1 <- commitment_signed l2 (using new feerate)
5. l1 -> revoke_and_ack l2
When we break the connection after #3, the reconnection causes #4 to
be retransmitted, but it turns out l1 wasn't telling the master to set
the local feerate until it received the commitment_signed, so on
reconnect it uses the old feerate, with predictable results (bad
signature).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Now sending a ping makes sense: it should force the other end to send
a reply, unblocking the commitment process.
Note that rather than waiting for a reply, we're actually spinning on
a 100ms loop in this case. But it's simple and it works.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This doesn't do much (though we might get an error before we send the
commitment_signed), but it's infrastructure for the next patch.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We were adding channels without their capacity, and eventually annotated them
when we exchanged `channel_update`s. This worked as long as we weren't
considering the channel capacity, but would result in local-only channels to be
unusable once we start checking.
Also means we simplify the handle_gossip_msg() since everyone wants it to
use sync_crypto_write().
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This is clearer and neater, and even slightly more efficient, since
read_peer_msg() was calling poll() again on gossipfd.
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>
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>
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>
That was the cause of the bad gossip order failures: gossipd thought our
channel was live, but the other end didn't receive message last time.
Now gossipd doesn't use fd to kill us (connectd tells master to do so), we
can implement read_peer_msg_nogossip().
Fixes: #1706
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This will avoid us having to round-trip to the HSM each time we want it.
For now we still derive it, too, and assert it's correct.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Well, it's generated by shachain, so technically it is a sha256, but
that's an internal detail. It's a secret.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
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>
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>
This resolves the problem where both channeld and gossipd can generate
updates, and they can have the same timestamp. gossipd is always able
to generate them, so can ensure timestamp moves forward.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Instead of considering it a temporary step, consider it a necessary preamble
to sending updates.
This means (in the next patch) when we tell gossipd to generate the updates,
it's always done after we've told it to create the channel.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
If we hit depth 6, we would start exchanging announcement signatures.
However, we should still send a temporary update while waiting for the
reply; make the logic clear in this case that we should always send
one or the other.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
The condition in send_channel_update is wrong: it needs to match the
conditions under which we send announcements.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Note: this will break the gossip_store if they have current channels,
but it will fail to parse and be discarded.
Have local_add_channel do just that: the update is logically separate
and can be sent separately.
This removes the ugly 'bool add_to_store' flag.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We always call:
send_temporary_announcement(peer);
send_announcement_signatures(peer);
We should handle these in one place, since the conditional at the top
of them actually makes sure only one is effective. We also make the
caller set the peer->have_sigs[LOCAL] flag, instead of doing it
inside send_announcement_signatures().
We were sending announcements at the wrong time (on restart) somtimes.
We also move announce_channel() into the same logic, so it's always
together.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Rusty Russell
Christian Decker
practicalswift
nicolas.dorier