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>
I saw this message:
```
lightning_channeld: outstanding taken(): channeld/channeld.c:3087:blinding
lightning_channeld: outstanding taken(): channeld/channeld.c:3087:blinding
lightning_channeld: outstanding taken(): channeld/channeld.c:3087:blinding
```
The caller does take(blinding), but blinding can be NULL. We should
move the code around to do the take() earlier anyway.
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.
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>
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>
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>
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)
We didn't care, but other implementations (particularly lnd) do. And it
does violate the spec.
(We need to use skip not xfail on the test which catches this, since
xfail doesn't seem to stop errors reported by cleanup)
(Includes Christian's typo fix!)
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
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>
dual funding needs the max-witness-len and utxo fields set for every
input. we should add them when we create a 'fundpsbt', so that every
psbt that c-lightning generates is dual-funding ready
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
There's a lot of it, and it means we can't `make check-source` on
these files.
Also bring bolt quotes up-to-date.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
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>
We're going to make experimental versions of these completely separate files.
Also remove the dependency on the Makefile itself: it simply causes
unnecessary churn. We can always force-rebuild when we change a rule.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We create ALL_PROGRAMS, ALL_TEST_PROGRAMS, ALL_C_SOURCES and
ALL_C_HEADERS. Then the toplevel Makefile knows which are
autogenerated (by wildcard), so it can have all the rules to clean
them or check the source as necessary.
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 also means we subtract 660 satoshis more everywhere we subtract
the base fee (except for mutual close, where the base fee is still
used).
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>
HTLC fees increase (larger weight), and the fee paid by the opener
has to include the anchor outputs (i.e. 660 sats).
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>
Update the `bitcoin_tx_add_input` interface to accept a witness script
and or scriptPubkey.
We save the amount + witness script + witness program (if known) to
the PSBT object for a transaction when creating an input.
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!
Since we now over-write the wally malloc/free functions, we need to do
so for tests as well. Here we pull up all of the common setup/teardown
logic into a separate place, and update the tests that use libwally to
use the new common_setup core
Changelog-None
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'
1. Use status_debug() instead of status_unusual() for when we can't make a
penalty tx (this happens easily the time if we fund a channel).
2. Use status_failed() (which exits) instead of status_broken() (which doesn't!)
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
`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>
We roll the `elements_add_fee_output` function and the cropping of
overallocated arrays into the `bitcoin_tx_finalize` function. This is supposed
to be the final cleanup and compaction step before a tx can be sent to bitcoin
or passed off to other daemons.
This is the cleanup promised in #3491
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>
Add new check if we're funder trying to add HTLC, keeping us
with enough extra funds to pay for another HTLC the peer might add.
We also need to adjust the spendable_msat calculation, and update
various tests which try to unbalance channels. We eliminate
the now-redundant test_channel_drainage entirely.
Changelog-Fixed: Corner case where channel could become unusable (https://github.com/lightningnetwork/lightning-rfc/issues/728)
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This sets the nLockTime to the tip (and accordingly each input's nSequence to
0xfffffffe) for withdrawal transactions.
Even if the anti fee-sniping argument might not be valid until some time yet,
this makes our regular wallet transactions far less distinguishable from
bitcoind's ones since it now defaults to using native Segwit transactions
(like us). Moreover other wallets are likely to implement this (if they
haven't already).
Changelog-Added: wallet: withdrawal transactions now sets nlocktime to the current tip.
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>
We still close the channel if we *send* an error, but we seem to have hit
another case where LND sends an error which seems transient, so this will
make a best-effort attempt to preserve our channel in that case.
Some test have to be modified, since they don't terminate as they did
previously :(
Changelog-Changed: quirks: We'll now reconnect and retry if we get an error on an established channel. This works around lnd sending error messages that may be non-fatal.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Thanks to @t-bast, who made this possible by interop testing with Eclair!
Changelog-Added: Protocol: can now send and receive TLV-style onion messages.
Changelog-Added: Protocol: can now send and receive BOLT11 payment_secrets.
Changelog-Added: Protocol: can now receive basic multi-part payments.
Changelog-Added: RPC: low-level commands sendpay and waitsendpay can now be used to manually send multi-part payments.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This is the final step: we pass the complete fee_states to and from
channeld.
Changelog-Fixed: "Bad commitment signature" closing channels when we sent back-to-back update_fee messages across multiple reconnects.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
These used to be necessary as we could have feerate changes which
we couldn't track: now we do, we don't need these flags.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This is an intermediary step: we still don't save it to the database,
but we do use the fee_states struct to track it internally.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
The `channel_got_commitsig` we send the lightningd also implies we sent
the revoke_and_ack, as an optimization. It doesn't currently matter,
since channel_sending_revoke_and_ack doesn't do anything important to the
state, but that changes once we start uploading the entire fee_states.
So now we move our state machine *before* sending to lightningd, in
preparation for sending fee_states too.
Unfortunately, we need to marshall the info to send before we
increment the state, as lightningd expects that.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This uses the same state machine as HTLCs, but they're only
ever added, not removed. Since we can only have one in each
state, we use a simple array; mostly NULL.
We could make this more space-efficient by folding everything into the
first 5 states, but that would be more complex than just using the
identical state machine.
One subtlety: we don't send uncommitted fee_states over the wire.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Now "raw_payload" is always the complete string (including realm or length
bytes at the front).
This has several effects:
1. We can receive an decrypt an onion which is grossly malformed.
2. We can still hand this to the htlc_accepted hook.
3. We then fail it unless the htlc_accepted accepts it manually.
4. The createonion API now takes the raw payload, and does not know
anything about "style".
The only caveat is that the sphinx code needs to know the payload
length: we have a call for that, which simply tells it to copy the
entire onion (and treat us as the final node) if it's invalid.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Also pulls in a new onion error (mpp_timeout). We change our
route_step_decode_end() to always return the total_msat and optional
secret.
We check total_amount (to prohibit mpp), but we do nothing with
secret for now other than hand it to the htlc_accepted hook.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Travis randomly picked up an error in test_feerate_stress:
**BROKEN** 0266e4598d1d3c415f572a8488830b60f7e744ed9235eb0b1ba93283b315c03518-channeld-chan#1: Cannot add htlc #0 10000msat to LOCAL (version a2541b9-modded)
This is because it hit an unlikely corner case involving applying multiple HTLCs
(similar to the previous c96cee9b8d).
In this case, the test sends a 500,000,000 "balancing" setup payment L1->L2.
It waits for L2 to get the preimage (which is the when pay() helper returns),
but crucially, it starts spamming with HTLCs before that HTLC is completely
removed.
From L2's point of view, the setup HTLC is in state RCVD_REMOVE_REVOCATION;
gone from L1's commitment tx, but still waiting for the commitment_signed
from L1 to remove it from L2's.
Note that each side keeps a local and remove view of both sides' current
balances: at this point, L2's view is REMOTE: "500,000,000 to L1, 499,900,000
to L2", LOCAL: "500,000,000 to L1, 0 to L2".
L2 sends a 10,000 msat HTLC to L1: legal, since L1 will allow it,
then the commitment_signed. L1 sends the revoke-and-ack for this,
*then* belatedly follows with the commitment_signed which both completes the
removal of the setup HTLC and adds the new one.
But L2 processes the HTLCs in hashtable (i.e. random) order: so if it
tries to apply its own HTLC first, it freaks out because it doesn't have
funds in its local view.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Fixed: Unlikely corner case is simultanous HTLCs near balance limits fixed.
This is ignored in subdaemons which are per-peer, but very useful for
multi-peer daemons like connectd and gossipd.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
A long time ago (93dcd5fed7), I
simplified the htlc reload code so it adjusted the amounts for HTLCs
in id order. As we presumably allowed them to be added in that order,
this avoided special-casing overflow (which was about to deliberately
be made harder by the new amount_msat code).
Unfortunately, htlc id order is not canonical, since htlc ids are
assigned consecutively in both directions! Concretely, we can have two HTLCs:
HTLC #0 LOCAL->REMOTE: 500,000,000 msat, state RCVD_REMOVE_REVOCATION
HTLC #0 REMOTE->LOCAL: 10,000 msat, state SENT_ADD_COMMIT
On a new remote-funded channel, in which we have 0 balance, these
commits *only* work in this order. Sorting by HTLC ID is not enough!
In fact, we'd have to worry about redemption order as well, as that
matters.
So, regretfully, we offset the balances halfway to UINT64_MAX, then check
they didn't underflow at the end. This loses us this one sanity check,
but that's probably OK.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Check behavior for user supplied upfront_shutdown_script via close_to
Header from folded patch 'fix__return__not__iff_well_close_to_the_provided_addr.patch':
fix: return not iff we'll close to the provided addr
I had a report of a 0.7.2 user whose node hadn't appeared on 1ml. Their
node_announcement wasn't visible to my node, either.
I suspect this is a consequence of recent version reducing the amount of
gossip they send, as well as large nodes increasingly turning off gossip
altogether from some peers (as we do). We should ignore timestamp filters
for our own channels: the easiest way to do this is to push them out
directly from gossipd (other messages are sent via the store).
We change channeld to wrap the local channel_announcements: previously
we just handed it to gossipd as for any other gossip message we received
from our peer. Now gossipd knows to push it out, as it's local.
This interferes with the logic in tests/test_misc.py::test_htlc_send_timeout
which expects the node_announcement message last, so we generalize
that too.
[ Thanks to @trueptolmy for bugfix! ]
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This is mainly an internal-only change, especially since we don't
offer any globalfeatures.
However, LND (as of next release) will offer global features, and also
expect option_static_remotekey to be a *global* feature. So we send
our (merged) feature bitset as both global and local in init, and fold
those bitsets together when we get an init msg.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Command format: close id [unilateraltimeout] [destination]
Close the channel with peer {id}, forcing a unilateral
close after {unilateraltimeout} seconds if non-zero, and
the to-local output will be sent to {destination}. If
{destination} isn't specified, the default is the address
of lightningd.
Also change the pylightning:
update the `close` API to support `destination` parameter
WIRE_REQUIRED_CHANNEL_FEATURE_MISSING anticipates a glorious Wumbo future,
and is closer to correct (it's a PERM failure).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We now have a pointer to chainparams, that fails valgrind if we do anything
chain-specific before setting it.
Suggested-by: Rusty Russell <@rustyrussell>
Elements requires us to have an explicit fee output instead of bitcoin's
implied fee. We add the fee output mostly after sorting the other outputs
since that matches the behavior in elements itself.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
It's generally clearer to have simple hardcoded numbers with an
#if DEVELOPER around it, than apparent variables which aren't, really.
Interestingly, our pruning test was always kinda broken: we have to pass
two cycles, since l2 will refresh the channel once to avoid pruning.
Do the more obvious thing, and cut the network in half and check that
l1 and l3 time out.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
531c8d7d9b
In this one, we always send my_current_per_commitment_point, though it's
ignored. And we have our official feature numbers.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
The largest change is inside hsmd: it hands a null per-commitment key
to the wallet to tell it to spend the to_remote output.
It can also now resolve unknown commitments, even if it doesn't have a
possible_remote_per_commitment_point from the peer.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
As per BOLT02 #message-retransmission :
if `next_commitment_number` is 1 in both the `channel_reestablish` it sent and received:
- MUST retransmit `funding_locked`
Rather than reaching into data structures, let them register their own
callbacks. This avoids us having to expose "memleak_remove_xxx"
functions, and call them manually.
Under the hood, this is done by having a specially-named tal child of
the thing we want to assist, containing the callback.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This removes the WIRE_FINAL_EXPIRY_TOO_SOON which leaked too much info,
and adds the blockheight to WIRE_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This is the normal convention for this type; it makes using converters
a little easier. See next patch.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We currently send channel_announcement as soon as we and our
peer agree it's 6 blocks deep. In theory, our other peers might
not have seen that block yet though, so delay a little.
This is mitigated by two factors:
1. lnd will stash any "not ready yet" channel_announcements anyway.
2. c-lightning doesn't enforce the 6 depth minimum at all.
We should not rely on other nodes' generosity or laxity, however!
Next release, we can start enforcing the depth limit, and maybe stashing
ones which don't quite make it (or simply enforce depth 5, not 6).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
The way we build transactions, serialize them, and compute fees depends on the
chain we are working on, so let's add some context to the transactions.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
And clean up some dev ones which actually happen (mainly by calling
channel_fail_permanent which logs UNUSUAL, rather than
channel_internal_error which logs BROKEN).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
If we ever do this, we'd end up with an unspendable commitment tx anyway.
It might be able to happen if we have htlcs added from the non-fee-paying
party while the fees are increased, though. But better to close the
channel and get a report about it if that happens.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We track whether each change is affordable as we go;
test_channel_drainage got us so close that the difference mattered; we
hit an assert when we tried to commit the tx and realized we couldn't
afford it.
We should not be trying to add an HTLC if it will result in the funder
being unable to afford it on either the local *or remote* commitments.
Note the test still "fails" because it refuses to send the final
payment.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Subtracting both arbitrarily reduces our capacity, even for ourselves
since the routing logic uses this maximum.
I also changed 'advertise' to 'advertize', since we use american
spelling.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@pm47 gave a great bug report showing c-lightning sending the same
UPDATE_FEE over and over, with the final surprise result being that we
blamed the peer for sending us multiple empty commits!
The spam is caused by us checking "are we at the desired feerate?" but
then if we can't afford the desired feerate, setting the feerate we
can afford, even though it's a duplicate. Doing the feerate cap before
we test if it's what we have already eliminates this.
But the empty commits was harder to find: it's caused by a heuristic in
channel_rcvd_revoke_and_ack:
```
/* For funder, ack also means time to apply new feerate locally. */
if (channel->funder == LOCAL &&
(channel->view[LOCAL].feerate_per_kw
!= channel->view[REMOTE].feerate_per_kw)) {
status_trace("Applying feerate %u to LOCAL (was %u)",
channel->view[REMOTE].feerate_per_kw,
channel->view[LOCAL].feerate_per_kw);
channel->view[LOCAL].feerate_per_kw
= channel->view[REMOTE].feerate_per_kw;
channel->changes_pending[LOCAL] = true;
}
```
We assume we never send duplicates, so we detect an otherwise-empty
change using the difference in feerates. If we don't set this flag,
we will get upset if we receive a commitment_signed since we consider
there to be no changes to commit.
This is actually hard to test: the previous commit adds a test which
spams update_fee and doesn't trigger this bug, because both sides
use the same "there's nothing outstanding" logic.
Fixes: #2701
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
