When receiving an update_fulfill_htlc message, we immediately
forward the claim backwards along the payment path before waiting
for a full commitment_signed dance. This is great, but can cause
duplicative claims if a node sends an update_fulfill_htlc message,
disconnects, reconnects, and then has to re-send its
update_fulfill_htlc message again.
While there was code to handle this, it treated it as a channel
error on the inbound channel, which is incorrect - this is an
expected, albeit incredibly rare, condition. Instead, we handle
these double-claims correctly, simply ignoring them.
With debug_assertions enabled, we also check that the previous
close of the same HTLC was a fulfill, and that we are not moving
from a HTLC failure to an HTLC claim after its too late.
A test is also added, which hits all three failure cases in
`Channel::get_update_fulfill_htlc`.
Found by the chanmon_consistency fuzzer.
This adds four new fields in `ChannelDetails`:
1. holder_selected_ and counterparty_selected_channel_reserve_delay
are useful to determine what amount of the channel is
unavailable for payments.
2. confirmations_required is useful when awaiting funding
confirmation to determine how long you will need to wait.
3. to_self_delay is useful to determine how long it will take to
receive funds after a force-close.
Fixes#983.
These fields are set with a dummy value, which we should generally
be avoiding since Rust gives us a nice `Option` type to use
instead.
Further, we stop rejecting channel_update messages outright when
the htlc_maximum_msat field includes the reserve values, which
nodes could reasonably do without it meriting a channel closure.
We had a user who pointed out that we weren't creating
`SpendableOutputs` events when we should have been after they
called `ChannelMonitor::best_block_updated` with a block well
after a CSV locktime and then called
`ChannelMonitor::transactions_confirmed` with the transaction which
we should have been spending (with a block height/hash a ways in
the past).
This was due to `ChannelMonitor::transactions_confirmed` only
calling `ChannelMonitor::block_confirmed` with the height at which
the transactions were confirmed, resulting in all checks being done
against that, not the current height.
Further, in the same scenario, we also would not fail-back and HTLC
where the HTLC-Timeout transaction was confirmed more than
ANTI_REORG_DELAY blocks ago.
To address this, we use the best block height for confirmation
threshold checks in `ChannelMonitor::block_confirmed` and pass both
the confirmation and current heights through to
`OnchainTx::update_claims_view`, using each as appropriate.
Fixes#962.
No matter the context, if we're told about a block which is
guaranteed by our API semantics to be on the best chain, and it has
a higher height than our current understanding of the best chain,
we should update our understanding. This avoids complexity
in `block_confirmed` by never having a height set which is *higher*
than our current best chain, potentially avoiding some bugs in the
rather-complicated code.
It also requires a minor test tweak as we in some cases now no
longer broadcast a conflicting transaction after the original has
reached the ANTI_REORG_DELAY.
There are no visible effects of this, but it seems like good code
hygiene to not call a disconnect function in a different file if no
disconnect happened.
We use `Channel::is_live()` to gate inclusion of a channel in
`ChannelManager::list_usable_channels()` and when sending an
HTLC to select whether a channel is available for
forwarding through/sending to.
In both of these cases, we should consider a channel `is_live()` when
they are pending a monitor update. Some clients may update monitors
asynchronously, thus we may simply be waiting a short duration for a
monitor update to complete, and shouldn't fail all forwarding HTLCs
during that time.
After #851, we always ensure any holding cells are free'd when
sending P2P messages, making this change much more trivially
correct - instead of having to ensure that we always free the holding
cell when a channel becomes live again after adding something to the
holding cell, we can simply rely on the fact that it always happens.
Fixes#661.
If we receive a `channel_update` message for a channel unrelated to
our own, we shouldn't trigger a persistence of our
`ChannelManager`. This avoids significant persistence traffic during
initial node startup.
We changed the sort order of log levels to be more natural, but this
comparison wasn't updated accordingly. Likely the reason it was
left strange for so long is it also had the comparison argument
ordering flipped.
ChannelMonitor and related log entries can generally lean towards
being higher log levels than they necessarily need to be, as they
should be exceedingly rare, if only because they require
confirmation of an on-chain transaction.
This updates a number of log sites in channel and channelmanager to
* Be a bit more verbose at the TRACE level,
* Move some error/useful messages to the ERROR/WARN/INFO level,
* Add new logs to always log once at the DEBUG level when we
send/receive a commitment_signed (with some extra data),
* Include the channel id being operated on in more log messages.
We very often receive duplicate gossip messages, which now causes us
to log at the DEBUG level, which is almost certainly not what a
user wants. Instead, we add a new form of ErrorAction which causes
us to only log at the TRACE level.
For log entries which may have a variable level, we can't call an
arbitrary macro and need to be able to pass an explicit level. This
does so without breaking the compile-time disabling of certain log
levels.
Further, we "fix" the comparison order of log levels to make more
significant levels sort "higher", which implicitly makes more sense
than sorting "lower".
Finally, we remove the "Off" log level as no log entry should ever
be logged at the "Off" level - that would be nonsensical.
This much more consistently logs information about messages
sent/received, including logging the full messages being
sent/received at the TRACE log level. Many other log messages which
are more often of interest were moved to the DEBUG log level.
We had a client application which provided inconsistent monitor
state when deserializing a ChannelManager, resulting in opaque and
generic "InvalidData" deserialization failures. Instead, we log
some informative (and appropriately scary) warning messages in
such cases.
`Event::PaymentSent` serialization has a bug where we
double-serialize the payment_preimage field but do not attempt to
read it twice. This results in a failure to read `ChannelManager`s
from disk if we have a pending `Event::PaymentSent` pending
awaiting handling when we serialize.
Instead of attempting to read both versions, we opt to simply fix
the serialization, assuming it is exceedingly rare for such a
scenario to appear (and if it does, we can assist in manual
recovery).
The release notes have been updated to note this inconsistency.
Found by the chanmon_consistency fuzz target.
lnd has a long-standing bug where, upon reconnection, if the
channel is not yet confirmed they will not send a
channel_reestablish until the channel locks in. Then, they will
send a funding_locked *before* sending the channel_reestablish
(which is clearly a violation of the BOLT specs). We copy
c-lightning's workaround here and simply store the funding_locked
message until we receive a channel_reestablish.
See-also https://github.com/lightningnetwork/lnd/issues/4006Fixes#963
Previous to this PR, TLV serialization involved iterating from 0 to the highest
given TLV type. This worked until we decided to implement keysend, which has a
TLV type of ~5.48 billion.
So instead, we now specify the type of whatever is being (de)serialized (which
can be an Option, a Vec type, or a non-Option (specified in the serialization macros as "required").
There are ~zero functions in lightning-invoice that are materially
callable from C, so there isn't any reason to tag it as a cdylib
(and make rustc build it as such). Instead, we have C bindings now.
