Looks like #4394 treated a symptom but not the root cause. We were
actually sending the message framed with the WIRE_CUSTOMMSG_OUT and
the length prefix over the encrypted connection to the peer. It just
happened to be a valid custommsg...
This fixes the issue, and this time I made sure we actually send the
raw message over the wire. However for backward compatibility we
needed to imitate the faulty behavior which is 90% of this patch :-)
Changelog-Fixed: plugin: `dev-sendcustommsg` included the type and length prefix when sending a message.
We move over to the new "warning" paradigm, instead of using
an "rbf_fail" message.
Every failure is either a warning or an error; on warnings we
hang up and reconnect later, effectively resetting the state.
And make all the callers choose which one. In general, I prefer warn,
which lets them reconnect and try again, however some places are either
stated that they must be errors in the spec itself, or in openingd
where we abandon the channel when we close the connection anyway.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Changed: Protocol: we now send warning messages and close the connection, except on unrecoverable errors.
This takes from the draft spec at https://github.com/lightningnetwork/lightning-rfc/pull/834
Note that if this draft does not get included, the peer will simply
ignore the warning message (we always close the connection afterwards
anyway).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Added: Protocol: we now report the new (draft) warning message.
Don't include exp directly, use an ifdef in common/bolt12
(like we do for peer and onion wiregen files).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
There's a case where a dropped funding_locked will result in the peer
moving onto channeld, while we stay in dualopend. As we haven't
received their funding_locked, we retransmit tx_sigs, which channeld
will need to handle.
With the patch the peer drops it on the floor; the peer will resend
funding_locked on reconnect, which will correctly advance us to
channeld and CHANNELD_NORMAL
We used this for dual funded opens, to track the receipt of signatures.
We're moving all of this over to dualopend now, however, so we no longer
need the PSBT in channeld.
The previous onion_message code required a confirmed, not-shutting-down
channel, not just a connection. That's overkill; plus before widespread
adoption we will want to connect directly as a last resort.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Still asserts that it's the standard size, but makes it a dynamic
member. For simpliciy, changes the parse_onionpacket API (it must be
a tal object now, so we might as well allocate it here to catch all
the callers).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
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.
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>
```
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>
libwally has a quirk where the finalize method will fail to 'completely'
finalize an input's parts if either the final_scriptsig or
final_redeemscript fields are set
since we manually set the final_witness stack here, we also need to
fully finalize the redeemscript -> final_scriptsig here as well.
Note that check-whitespace and check-bolt already do this, so we
can eliminate redundant lines in common/Makefile and bitcoin/Makefile.
We also include the plugin headers in ALL_C_HEADERS so they get
checked.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We used to send our tx_sigs before we got to channeld existing. We
changed how this worked so that multifundchannel could live, but failed
to clean up the logic of what "having a psbt around" means wrt channeld
and messaging our peer.
The general idea is that we want to send `tx_signatures` to our peer on
reconnect until they've sent us `funding_locked`.
Note that it's an error to
- send `funding_locked` without having sent `tx_signatures`
- send `tx_signatures` after sending `funding_locked`
We use the 'finalized' state of the peer's inputs/outputs to help signal
where we are in receiving their sigs -- but this doesn't work at all for
opens where the peer doesn't contribute inputs at all.
This isn't really a huge deal, but it does mean that if we receive a
peer's `tx_sigs` more than once (will happen for a reconnect before
`funding_locked`), then we'll issue a notification about receiving their
sigs multiple times. /shrug
Prior to this patch update, we expected a client to call
`openchannel_signed` before checking for peer's tx-sigs messages on the
wire.
When moving to a 'multifundchannel' approach, we'll need to be able to
collect sigs from our peers before sending our tx_sigs message. There's
no strict ordering on when tx-sigs messages are sent/received, so this
is fine.
To do this, we go ahead and start up channeld as soon as
commitment_sigs are secured, so that we process incoming tx-sigs from
our peers as soon as we get them.
We're about to totally upset the order that sigs are set on our PSBTs
for new channel opens, making it such that our peer's sigs may arrive
before ours do.
We can no longer rely on the 'set witness means this is our input' since
there's no guarantee that our input sigs have been added yet, so we
check the serial_id and only set the stack on their (odd) inputs.
Instead of a boutique message, use a "real" channel_announcement for
private channels (with fake sigs and pubkeys). This makes it far
easier for gossmap to handle local channels.
Backwards compatible update, since we update old stores.
We also fix devtools/dump-gossipstore to know about the tombstone markers.
Since we increment our channel_announce count for local channels now,
the stats in the tests changed too.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
There's a few structs/wire calls that only exist under experimental features.
These were in a common file that was shared/used a bunch of places but
this causes problems. Here we move one of the problematic methods back
into `openingd`, as it's only used locally and then isolate the
references to the `witness_stack` in a new `common/psbt_internal` file.
This lets us remove the iff EXP_FEATURES inclusion switches in most of
the Makefiles.
...right time.
We re-send the tx_sigs on start/init/reconnect until we've gotten a
funding_locked from our peer. We also build it in channeld now, instead
of in dualopend, and don't pass in a message for them anymore
We need the PSBT to create the finalized tx from once the peer's
tx_signatures are received. Since we're passing the PSBT, we no longer
need the secondary message to be passed, as it was derived from the
PSBT.
Also removes now unused witness serialization code
We had one report of this, and then Eugene and Roasbeef of Lightning
Labs confirmed it; they saw misordered HTLCs on reconnection too.
Since we didn't enforce this when we receive HTLCs, we never noticed :(
Fixes: #3920
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Fixed: Protocol: fixed retransmission order of multiple new HTLCs (causing channel close with LND)
1. Rename memleak_enter_allocations to memleak_find_allocations.
2. Unify scanning for pointers into memleak_remove_region / memleak_remove_pointer.
3. Document the functions.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
v2 of channel establishment, in the accpeter case, now sends 2 messages
to our peer after saving the information to disk (our commitment
signatures and our funding transaction signatures)
v2 channel open uses a different method to derive the channel_id, so now
we save it to the database so that we dont have to remember how to
derive it for each.
includes a migration for existing channels
This avoids overwriting the ones in git, and generally makes things neater.
We have convenience headers wire/peer_wire.h and wire/onion_wire.h to
avoid most #ifdefs: simply include those.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Fixes: #3608
Changelog-Changed: protocol: Ignore (and log as "unusual") repeated `WIRE_CHANNEL_REESTABLISH` messages, to be compatible with buggy peer software that sometimes does this.
Note that other directories were explicitly depending on the generated
file, instead of relying on their (already existing) dependency on
$(LIGHTNINGD_HSM_CLIENT_OBJS), so we remove that.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This means some files get renamed, and I took the opportunity to clarify
our naming (the *d* is important!)
1. channeld/channel_wire.csv -> channeld/channeld_wire.csv
2. channeld/gen_channel_wire.h -> channeld/channeld_wiregen.h
3. enum channel_wire_type -> enum channeld_wire
4. WIRE_CHANNEL_FUNDING_DEPTH -> WIRE_CHANNELD_FUNDING_DEPTH.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This is best done by passing `struct bitcoin_signature` around instead
of raw signatures. We still save raw sigs to the db, and of course the
wire protocol uses them.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Includes:
psbt: Use renamed functions for new wally version
psbt: Set the transaction directly to avoid script workarounds
psbt: Use low-S grinding when computing signatures
tx: Use wally_tx_clone from libwally now that its exported
Signed-off-by: Jon Griffiths <jon_p_griffiths@yahoo.com>
The main change here is that the previously-optional open/accept
fields and reestablish fields are now compulsory (everyone was
including them anyway). In fact, the open/accept is a TLV
because it was actually the same format.
For more details, see lightning-rfc/f068dd0d8dfa5ae75feedd99f269e23be4777381
Changelog-Removed: protocol: support for optioned form of reestablish messages now compulsory.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
now that witness script data is saved into the tx/psbt which is
serialized across the wire, there's no reason to use witscript to do
this. good bye witscript!
Adds a new plugin notification for getting information about coin
movements. Also includes two 'helper' notification methods that can be
called from within lightningd. Separated from the 'common' set because
the lightningd struct is required to finalize the blockheight etc
Changelog-Added: Plugins: new notification type 'coin_movement'
`lightningd` passes in all the known penalty_bases when starting a new
`channeld` instance, which tracks them internally, eventually matching them
with revocations and passing them back to `lightningd` so it can create the
penalty transaction. From here it is just a small step to having `channeld`
also generate the penalty transaction if desired.
When we have only a single member in a TLV (e.g. an optional u64),
wrapping it in a struct is awkward. This changes it to directly
access those fields.
This is not only more elegant (60 fewer lines), it would also be
more cache friendly. That's right: cache hot singles!
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Previously we've used the term 'funder' to refer to the peer
paying the fees for a transaction; v2 of openchannel will make
this no longer true. Instead we rename this to 'opener', or the
peer sending the 'open_channel' message, since this will be universally
true in a dual-funding world.
The peer will fail the channel on reconnect if we send an 'add' we
don't have balance for yet; we can avoid this issue by always sending
fulfills (+) before sending adds (-)
Note that it's channeld which calculates the shared secret, too. This
minimizes the work that lightningd has to do, at cost of passing this
through.
We also don't yet save the blinding field(s) to the database.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Sending update_fee immediately after channel establishment seems to
upset LND, so work around it by deferring it. The reason we increase
the fee after establishment is because now we might need to close the
channel in a hurry due to htlcs, but until there are htlcs that's
unnecessary.
Fixes: #3596
Changelog-Changed: Added workaround for lnd rejecting our commitment_signed when we send an update_fee after channel confirmed.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We don't free the signatures in this case, and for some reason leak checking
on my build machine just found it:
MEMLEAK: 0x560f7dc69fc8'
label=channeld/gen_channel_wire.c:266:secp256k1_ecdsa_signature'
backtrace:'
ccan/ccan/tal/tal.c:442 (tal_alloc_)'
channeld/gen_channel_wire.c:266 (fromwire_channel_init)'
channeld/channeld.c:3060 (init_channel)'
channeld/channeld.c:3254 (main)'
parents:'
channeld/channeld.c:3227:struct peer'
MEMLEAK: 0x560f7dc6a288'
label=channeld/gen_channel_wire.c:272:secp256k1_ecdsa_signature'
backtrace:'
ccan/ccan/tal/tal.c:442 (tal_alloc_)'
channeld/gen_channel_wire.c:272 (fromwire_channel_init)'
channeld/channeld.c:3060 (init_channel)'
channeld/channeld.c:3254 (main)'
parents:'
channeld/channeld.c:3227:struct peer'
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
common/onion is going to need to use this for the case where it finds a blinding
seed inside the TLV. But how it does ecdh is daemon-specific.
We already had this problem for devtools/gossipwith, which supplied a
special hsm_do_ecdh(). This just makes it more general.
So we create a generic ecdh() interface, with a specific implementation
which subdaemons and lightningd can use.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We currently abuse the added_htlc and failed_htlc messages to tell channeld
about existing htlcs when it restarts. It's clearer to have an explicit
'existing_htlc' type which contains all the information for this case.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
It's almost always "their_features" and "our_features" respectively, so
make those names clear.
Suggested-by: @cdecker
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Turns out that unnecessary: all callers can access the feature_set,
so make it much more like a normal primitive.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
At the moment, we store e.g. WIRE_TEMPORARY_CHANNEL_FAILURE, and then
lightningd has a large demux function which turns that into the correct
error message.
Such an enum demuxer is an anti-pattern.
Instead, store the message directly for output HTLCs; channeld now
sends us an error message rather than an error code.
For input HTLCs we will still need the failure code if the onion was
bad (since we need to prompt channeld to send a completely different
message than normal), though we can (and will!) eliminate its use in
non-BADONION failure cases.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We'll use this in the next patch for when we need to create errors to
send back to lightningd; most commonly when the channel doesn't have
capacity for the HTLC.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Instead of making it ourselves, lightningd does it. Now we only have
two cases of failed htlcs: completely malformed (BADONION), and with
an already-wrapped onion reply to send.
This makes channeld's job much simpler.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
For incoming htlcs, we need failure details in case we need to
re-xmit them. But for outgoing htlcs, lightningd is telling us it
already knows they've failed, so we just need to flag them failed
and don't need the details.
Internally, we set the ->fail to a dummy non-NULL value; this is
cleaned up next.
This matters for the next patch, which moves onion handling into
lightningd.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We could use sendonion to do this, but it actually takes a different path through
pay, and I wanted to test all of it, so I made a new dev flag.
We currently get upset with the response:
lightningd/pay.c:556: payment_failed: Assertion `!hout->failcode' failed.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
These messages may be exchanged between the master and any daemon. For now
these are just the daemons that a peer may be attached to at any time since
the first example of this is the custommsg infrastructure.
Generally I prefer structures over u8, since the size is enforced at
runtime; and in several places we were doing conversions as the code
using Sphinx does treat struct secret as type of the secret.
Note that passing an array is the same as passing the address, so
changing from 'u8 secret[32]' to 'struct secret secret' means various
'secret' parameters change to '&secret'. Technically, '&secret' also
would have worked before, since '&' is a noop on array, but that's
always seemed a bit weird.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This makes it clear we're dealing with a message which is a wrapped error
reply (needing unwrap_onionreply), not an already-wrapped one.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
