It is useful for accounting and informational reasons for users to
be informed when a payment has been successfully forwarded. Thus,
when an HTLC which represents a forwarded leg is claimed, we
generate a new `PaymentForwarded` event.
This requires some additional plumbing to return HTLC values from
`OnchainEvent`s. Further, when we have to go on-chain to claim the
inbound side of the payment, we do not inform the user of the fee
reward, as we cannot calculate it until we see what is confirmed
on-chain.
Substantial code structure rewrites by:
Valentine Wallace <vwallace@protonmail.com>
Previously, we could fail to generate a new commitment transaction
but it simply indicated we had gone to doule-claim an HTLC. Now
that double-claims are returned instead as Ok(None), we should
handle the error case and fail the channel, as the only way to hit
the error case is if key derivation failed or the user refused to
sign the new commitment transaction.
This also resolves an issue where we wouldn't inform our
ChannelMonitor of the new payment preimage in case we failed to
fetch a signature for the new commitment transaction.
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.
As the variable name implies holder_selected_chan_reserve_msat is
intended to be in millisatoshis, but is instead calculated in
satoshis.
We fix that error here and update the relevant tests to more
accurately calculate the expected reserve value and test both
success and failure cases.
Bug discovered by chanmon_consistency fuzz target.
Instead of interpreting the backwards compatibility data in Channel
serialization, use the serialization version bump present in 0.0.99
as the flag to indicate if a channel should be read in backwards
compatibility.
Currently the base fee we apply is always the expected cost to
claim an HTLC on-chain in case of closure. This results in
significantly higher than market rate fees [1], and doesn't really
match the actual forwarding trust model anyway - as long as
channel counterparties are honest, our HTLCs shouldn't end up
on-chain no matter what the HTLC sender/recipient do.
While some users may wish to use a feerate that implies they will
not lose funds even if they go to chain (assuming no flood-and-loot
style attacks), they should do so by calculating fees themselves;
since they're already charging well above market-rate,
over-estimating some won't have a large impact.
Worse, we current re-calculate fees at forward-time, not based on
the fee we set in the channel_update. This means that the fees
others expect to pay us (and which they calculate their route based
on), is not what we actually want to charge, and that any attempt
to forward through us is inherently race-y.
This commit adds a configuration knob to set the base fee
explicitly, defaulting to 1 sat, which appears to be market-rate
today.
[1] Note that due to an msat-vs-sat bug we currently actually
charge 1000x *less* than the calculated cost.
This was missed prior to 0.0.98, so requires a
backwards-compatibility wrapper inside the `Channel` serialization
logic, but it's not very complicated to do so.
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 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.
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.
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").
Channel serialization should happen "as if
remove_uncommitted_htlcs_and_mark_paused had just been called".
This is true for the most part, but outbound RemoteRemoved HTLCs
were being serialized as normal, even though
`remote_uncommitted_htlcs_and_mark_paused` resets them to
`Committed`.
This led to a bug identified by the `chanmon_consistency_target`
fuzzer wherein, if we receive a update_*_htlc message bug not the
corresponding commitment_signed prior to a serialization roundtrip,
we'd force-close the channel due to the peer "attempting to
fail/claim an HTLC which was already failed/claimed".
Previously we handled most of the logic of announcement_signatures
in ChannelManager, rather than Channel. This is somewhat unique as
far as our message processing goes, but it also avoided having to
pass the node_secret in to the Channel.
Eventually, we'll move the node_secret behind the signer anyway, so
there isn't much reason for this, and storing the
announcement_signatures-provided signatures in the Channel allows
us to recreate the channel_announcement later for rebroadcast,
which may be useful.
Currently our serialization is very compact, and contains version
numbers to indicate which versions the code can read a given
serialized struct. However, if you want to add a new field without
needlessly breaking the ability of previous versions of the code to
read the struct, there is not a good way to do so.
This adds dummy, currently empty, TLVs to the major structs we
serialize out for users, providing an easy place to put new
optional fields without breaking previous versions.
Previously, if we got disconnected from a peer while there were
HTLCs pending forwarding in the holding cell, we'd clear them and
fail them all backwards. This is largely fine, but since we now
have support for handling such HTLCs on reconnect, we might as
well not, instead relying on our timeout logic to fail them
backwards if it takes too long to forward them.
If there is no pending channel update messages when monitor updating
is restored (though there may be an RAA to send), and we're
connected to our peer and not awaiting a remote RAA, we need to
free anything in our holding cell.
However, we don't want to immediately free the holding cell during
channel_monitor_updated as it presents a somewhat bug-prone case of
reentrancy:
a) it would re-enter user code around a monitor update while being
called from user code notifying us of the same monitor being
updated, making deadlocs very likely (in fact, our fuzzers
would have a bug here!),
b) the re-entrancy only occurs in a very rare case, making it
likely users will not hit it in testing, only deadlocking in
production.
Thus, we add a holding-cell-free pass over each channel in
get_and_clear_pending_msg_events. This fits up nicely with the
anticipated bug - users almost certainly need to process new
network messages immediately after monitor updating has been
restored to send messages which were not sent originally when the
monitor updating was paused.
Without this, chanmon_fail_consistency was able to find a stuck
condition where we sit on an HTLC failure in our holding cell and
don't ever handle it (at least until we have other actions to take
which empty the holding cell).
Currently, we only send an update_channel message after
disconnecting a peer and waiting some time. We do not send a
followup when the peer has been reconnected for some time.
This changes that behavior to make the disconnect and reconnect
channel updates symmetric, and also simplifies the state machine
somewhat to make it more clear.
Finally, it serializes the current announcement state so that we
usually know when we need to send a new update_channel.
Current Bitcoin Core's policy will reject a p2wsh as a dust if it's
under 330 satoshis. A typical p2wsh output is 43 bytes big to which
Core's `GetDustThreshold()` sums up a minimal spend of 67 bytes (even
if a p2wsh witnessScript might be smaller). `dustRelayFee` is set
to 3000 sat/kb, thus 110 * 3000 / 1000 = 330. As all time-sensitive
outputs are p2wsh, a value of 330 sat is the lower bound desired
to ensure good propagation of transactions. We give a bit margin to
our counterparty and pick up 660 satoshis as an accepted
`dust_limit_satoshis` upper bound.
As this reasoning is tricky and error-prone we hardcode it instead of
letting the user picking up a non-sense value.
Further, this lower bound of 330 sats is also hardcoded as another constant
(MIN_DUST_LIMIT_SATOSHIS) instead of being dynamically computed on
feerate (derive_holder_dust_limit_satoshis`). Reducing risks of
non-propagating transactions in casee of failing fee festimation.
