Matt Corallo [Wed, 30 Jun 2021 03:16:01 +0000 (03:16 +0000)]
Limit inbound fee updates by dust exposure instead of our estimator
Inbound fee udpates are rather broken in lightning as they can
impact the non-fundee despite the funder paying the fee, but only
in the dust exposure it places on the fundee.
At least lnd is fairly aggressively high in their (non-anchor) fee
estimation, running the risk of force-closure. Further, because we
relied on a fee estimator we don't have full control over, we
were assuming our users' fees are particularly conservative, and
thus were at a lot of risk to force-closures.
This converts our fee limiting to use an absurd upper bound,
focusing on whether we are over-exposed to in-flight dust when we
receive an update_fee.
Matt Corallo [Mon, 12 Jul 2021 15:39:27 +0000 (15:39 +0000)]
Fix handling of inbound uncommitted feerate updates
If we receive an update_fee but do not receive a commitment_signed,
we should not persist the pending fee update to disk or hold on to
it after our peer disconnects.
In order to make the code the most readable, we add a state enum
which matches the relevant states from InboundHTLCState, allowing
for more simple code comparison between inbound HTLC handling and
update_fee handling.
Matt Corallo [Wed, 30 Jun 2021 18:12:51 +0000 (18:12 +0000)]
Fix re-sending commitment updates with an outbound fee update
When we send an update_fee to our counterparty on an outbound
channel, if we need to re-send a commitment update after
reconnection, the update_fee must be present in the re-sent
commitment update messages. However, wewere always setting the
update_fee field in the commitment update to None, causing us to
generate invalid commitment signatures and get channel
force-closures.
This fixes the issue by correctly detecting when an update_fee
needs to be re-sent, doing so when required.
Matt Corallo [Mon, 28 Jun 2021 03:41:44 +0000 (03:41 +0000)]
Automatically update fees on outbound channels as fees change
Previously we'd been expecting to implement anchor outputs before
shipping 0.1, thus reworking our channel fee update process
entirely and leaving it as a future task. However, due to the
difficulty of working with on-chain anchor pools, we are now likely
to ship 0.1 without requiring anchor outputs.
In either case, there isn't a lot of reason to require that users
call an explicit "prevailing feerates have changed" function now
that we have a timer method which is called regularly. Further, we
really should be the ones deciding on the channel feerate in terms
of the users' FeeEstimator, instead of requiring users implement a
second fee-providing interface by calling an update_fee method.
Finally, there is no reason for an update_fee method to be
channel-specific, as we should be updating all (outbound) channel
fees at once.
Thus, we move the update_fee handling to the background, calling it
on the regular 1-minute timer. We also update the regular 1-minute
timer to fire on startup as well as every minute to ensure we get
fee updates even on mobile clients that are rarely, if ever, open
for more than one minute.
Antoine Riard [Wed, 28 Jul 2021 23:55:11 +0000 (19:55 -0400)]
Enforce `max_balance_dust_htlc_msat` at HTLC reception/forward
At `update_add_htlc()`/`send_htlc()`, we verify that the inbound/
outbound dust or the sum of both, on either sides of the link isn't
above new config setting `max_balance_dust_htlc_msat`.
A dust HTLC is hence defined as a trimmed-to-dust one, i.e including
the fee cost to publish its claiming transaction.
Antoine Riard [Wed, 28 Jul 2021 23:51:02 +0000 (19:51 -0400)]
Add new config setting `max_balance_dust_htlc_msat`
Trimmed-to-dust HTLCs are at risk of being burnt as miner fees
at anytime during their lifetime due to the broadcast of either
holder commitment transaction or counterparty's one.
To hedge against this risk, we introduce a new config setting
`max_balance_dust_htlc_msat`, with the initial value of
5_000_000 msat.
Jeffrey Czyz [Mon, 9 Aug 2021 20:15:12 +0000 (15:15 -0500)]
Remove unreachable BroadcastChannelUpdate
When handling shutdown messages, Channel cannot move to
ChannelState::ShutdownComplete. Remove the code in ChannelManager that
adds a MessageSendEvent::BroadcastChannelUpdate in this case since it is
unreachable.
When a shutdown script is omitted from open_channel or accept_channel,
it must be provided when sending shutdown. Generate the shutdown script
at channel closing time in this case rather at channel opening.
This requires producing a ChannelMonitorUpdate with the shutdown script
since it is no longer known at ChannelMonitor creation.
KeysInterface::get_shutdown_pubkey is used to form P2WPKH shutdown
scripts. However, BOLT 2 allows for a wider variety of scripts. Refactor
KeysInterface to allow any supported script while still maintaining
serialization backwards compatibility with P2WPKH script pubkeys stored
simply as the PublicKey.
Add an optional TLV field to Channel and ChannelMonitor to support the
new format, but continue to serialize the legacy PublicKey format.
BOLT 2 enumerates the script formats that may be used for a shutdown
script. KeysInterface::get_shutdown_pubkey returns a PublicKey used to
form one of the acceptable formats (P2WPKH). Add a ShutdownScript
abstraction to encapsulate all accept formats and be backwards
compatible with P2WPKH scripts serialized as the corresponding
PublicKey.
Matt Corallo [Wed, 4 Aug 2021 16:21:36 +0000 (16:21 +0000)]
Suggest faster ping in `PeerManager::timer_tick_occurred` docs
This clarifies the docs for `PeerManager::timer_tick_occurred` to
note that the call rate is entirely up to the user, and also
suggests a faster ping rate of "once every five to ten seconds"
instead of "every 30 seconds". There isn't a lot of reason to want
to ping less often, and faster ping means we detect disconnects
sooner, which is important.
Matt Corallo [Sat, 31 Jul 2021 03:34:16 +0000 (03:34 +0000)]
Correctly detect missing HTLCs when a local commitment tx was broadcast
If we forward an HTLC to our counterparty, but we force-closed the
channel before our counterparty provides us an updated commitment
transaction, we'll end up with a commitment transaction that does
not contain the HTLC which we attempted to forward. In this case,
we need to wait `ANTI_REORG_DELAY` blocks and then fail back the
HTLC as there is no way for us to learn the preimage and the
confirmed commitment transaction paid us the value of the HTLC.
However, check_spend_holder_transaction did not do this - it
instead only looked for dust HTLCs in the confirmed commitment
transaction, paying no attention to what other HTLCs may exist that
are missed.
This will eventually lead to channel force-closure as the channel
on which we received the inbound HTLC to forward will be closed in
time for the initial sender to claim the HTLC on-chain.
Matt Corallo [Sat, 31 Jul 2021 03:31:10 +0000 (03:31 +0000)]
DRY HTLC failure code in check_spend_counterparty_transaction
This extracts the HTLC-not-in-broadcasted-commitment-transaction
code from check_spend_counterparty_transaction and moves it to a
global macro, DRYing up the two very similar codepaths (fixing
some minor logging inconsistencies) in the process.
This macro will be used for local commitment transaction HTLC
failure as well in the next commit.
This commit has no functional change outside of logging.
Matt Corallo [Sun, 1 Aug 2021 02:42:42 +0000 (02:42 +0000)]
Make BackgroundProcessor `#[must_use]` to avoid dropping immediately
It is easy for users to have a bug where they drop a
`BackgroundProcessor` immediately, causing it to start and then
immediately stop. Instead, add a `#[must_use]` tag to provide a
compiler warning for such instances.
Matt Corallo [Sun, 1 Aug 2021 02:13:36 +0000 (02:13 +0000)]
Log when a channel is closed on startup due to stale ChannelManager
This is one of the riskiest parts of our API from the perspective
of accidental force-closes - if users delay persisting the
ChannelManager much at all after a ChannelMonitor we may hit a
force-close after restart.
The fact that we don't log at all when this happens is criminal.
Matt Corallo [Fri, 16 Jul 2021 18:10:37 +0000 (18:10 +0000)]
Add additional TLV serialization type of (default_value, N)
This allows TLV serialization macros to read non-Option-wrapped
types but allow them to be missing, filling them in with the
provided default value as needed.
Matt Corallo [Tue, 3 Aug 2021 16:49:31 +0000 (16:49 +0000)]
Change return value of `claim_funds` to ignore duplicate claims
While we should never reach `ClaimFundsFromHop::DuplicateClaim` in
most cases, if we do, it likely indicates the HTLC was timed out
some time ago and is no longer available to be claimed. Thus, it
does not make sense to imply that we `claimed_any_htlcs`.
Matt Corallo [Fri, 16 Jul 2021 02:16:50 +0000 (02:16 +0000)]
Generate a PaymentForwarded event when a forwarded HTLC is claimed
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.
Matt Corallo [Thu, 29 Jul 2021 19:49:09 +0000 (19:49 +0000)]
Fix to_remote SpendableOutputs generation in rare reorg cases
If we first see a local commitment transaction, and then a reorg
causes the confirmed channel close transaction to instead be a
remote commitment transaction, we would fail a spurious `if else`
check, resulting in us not generating the correct `SpendableOutput`
event for the to_remote output now confirmed on chain.
This resolves the incorrect logic and adds a regression test.
Matt Corallo [Mon, 2 Aug 2021 15:04:40 +0000 (15:04 +0000)]
Check IO errors in test using `raw_os_error()` instead of `kind()`
std::io::ErrorKind is a `#[non_exhaustive]` enum as more specific
error types are to be added in the future. It was unclear in the
docs until very recently, however, that this is to be done by
re-defining `ErrorKind::Other` errors to new enum variants. Thus,
our tests which check explicitly for `ErrorKind::Other` as a
result of trying to access a directory as a file were incorrect.
Sadly, these generated no meaningful feedback from rustc at all,
except that they're suddenly failing in rustc beta!
After some back-and-forth, it seems rustc is moving forward
breaking existing code in future versions, so we move to the
"correct" check here, which is to check the raw IO error.
See rust-lang/rust#86442 and rust-lang/rust#85746 for more info.
The previous commit wraps the background thread's JoinHandle in an
Option. Providing a dedicated method to join hides this implementation
detail from users.
Matt Corallo [Fri, 16 Jul 2021 00:21:52 +0000 (00:21 +0000)]
Test preimages are learned instantly in test_onchain_to_onchain_claim
test_onchain_to_onchain_claim was connecting additional blocks in
order to reach HTLC timeout and broadcast an HTLC-Timeout
transaction, resulting in it not testing whether HTLC preimages are
learned instantly in response to HTLC-Success transactions.
Matt Corallo [Thu, 15 Jul 2021 16:30:52 +0000 (16:30 +0000)]
Ignore unknown Events serialized with an odd type value.
This should provide some additional future extensibility, allowing
for new informational events which can be safely ignored to be
ignored by older versions.
Matt Corallo [Thu, 15 Jul 2021 16:00:15 +0000 (16:00 +0000)]
Drop single-use macro from check_spend_holder_transaction
The wait_threshold_conf!() macro in check_spend_holder_transaction
was only used once, making it a good candidate for inlining at the
callsite. Further, it incorrectly always logged that we were
failing HTLCs from the "latest" commitment transaction, when it is
sometimes actually failing HTLCs from the previous commitment
transaction.
Matt Corallo [Thu, 15 Jul 2021 22:26:51 +0000 (22:26 +0000)]
Fail channel if we can't sign a new commitment tx during HTLC claim
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.
Matt Corallo [Tue, 29 Jun 2021 21:05:45 +0000 (21:05 +0000)]
Handle double-HTLC-claims without failing the backwards channel
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`.
Without stopping the thread when BackgroundProcessor is dropped, it will
run free. In the context of language bindings, it is difficult to know
how long references held by the thread should live. Implement Drop to
stop the thread just as is done when explicitly calling stop().
The specific error from the ChannelManager persister is not asserted for
in test_persist_error. Rather, any error will do. Update the test to use
BackgroundProcessor::stop and assert for the expected value.