rust-lightning

Rebuild pending payments list before replaying pending claims/fails

On `ChannelManager` reload we rebuild the pending outbound payments
list by looking for any missing payments in `ChannelMonitor`s.
However, in the same loop over `ChannelMonitor`s, we also re-claim
any pending payments which we see we have a payment preimage for.

If we send an MPP payment across different chanels, the result may
be that we'll iterate the loop, and in each iteration add a
pending payment with only one known path, then claim/fail it and
remove the pending apyment (at least for the claim case). This may
result in spurious extra events, or even both a `PaymentFailed` and
`PaymentSent` event on startup for the same payment.

`rustfmt` and clean up `get_onchain_failed_outbound_htlcs`

Re-fail perm-failed HTLCs on startup in case of `MonitorEvent` loss

`MonitorEvent`s aren't delivered to the `ChannelManager` in a
durable fashion - if the `ChannelManager` fetches the pending
`MonitorEvent`s, then the `ChannelMonitor` gets persisted (i.e. due
to a block update) then the node crashes, prior to persisting the
`ChannelManager` again, the `MonitorEvent` and its effects on the
`ChannelManger` will be lost. This isn't likely in a sync persist
environment, but in an async one this could be an issue.

Note that this is only an issue for closed channels -
`MonitorEvent`s only inform the `ChannelManager` that a channel is
closed (which the `ChannelManager` will learn on startup or when it
next tries to advance the channel state), that
`ChannelMonitorUpdate` writes completed (which the `ChannelManager`
will detect on startup), or that HTLCs resolved on-chain post
closure. Of the three, only the last is problematic to lose prior
to a reload.

In a previous commit we handled the case of claimed HTLCs by
replaying payment preimages on startup to avoid `MonitorEvent` loss
causing us to miss an HTLC claim. Here we handle the HTLC-failed
case similarly.

Unlike with HTLC claims via preimage, we don't already have replay
logic in `ChannelManager` startup, but its easy enough to add one.
Luckily, we already track when an HTLC reaches permanently-failed
state in `ChannelMonitor` (i.e. it has `ANTI_REORG_DELAY`
confirmations on-chain on the failing transaction), so all we need
to do is add the ability to query for that and fail them on
`ChannelManager` startup.

Add clarifying comment to signer_maybe_unblocked

Include fees in SpliceContribution docs

How fees are paid for in a SpliceContribution depends on whether it is a
SpliceIn or SpliceOut. Include this in its docs for clarification.

Fix debug_assert on our_funding_contribution

When processing a splice_ack, the debug_assert on the range of
our_funding_contribution should account for values that are for both
positive (splice-in) and negative (splice-out).

Add splice-out support

Update SpliceContribution with a variant used to support splice-out
(i.e., removing funds from a channel). The TxOut values must not exceed
the users channel balance after accounting for fees and the reserve
requirement.

Support accepting splice-out

When a counterparty sends splice_init with a negative contribution, they
are requesting to remove funds from a channel. Remove conditions
guarding against this and check that they have enough channel balance to
cover the removed funds.

Use a SpliceContribution enum for passing splice-in params

ChannelManager::splice_channel takes individual parameters to support
splice-in. Change these to an enum such that it can be used for
splice-out as well.

Replace funding input tuple with struct

The funding inputs used for splicing and v2 channel establishment are
passed as a tuple of txin, prevtx, and witness weight. Add a struct so
that the items included can be better documented.

Rename ChannelContext::counterparty_prev_commitment_point

To align with the "current" and "next" nomenclature used by
HolderCommitmentPoint, update the naming of the counterparty commitment
point field to use "current" instead of "previous".

Rename ChannelContext::counterparty_cur_commitment_point

To align with the "current" and "next" nomenclature used by
HolderCommitmentPoint, update the naming of the counterparty commitment
point field to use "next" instead of "current".

Rename ChannelContext::cur_counterparty_commitment_transaction_number

To align with the "current" and "next" nomenclature used by
HolderCommitmentPoint, update the naming of the counterparty commitment
transaction number field to use "next" instead of "current".

Return ChannelError instead of calling expect

Delete dead `next_{local, remote}_commitment_tx_fee_info_cached`

The cached fee is never checked in the current test suite.

Add validation of the fees predicted by `next_commitment_stats`

Anytime we build a (feerate, nondust-htlc-count, fee) pair, cache it,
and check that the fee matches if the feerate and nondust-htlc-count
match when building a commitment transaction.

Add `ChannelContext::get_next_{local, remote}_commitment_stats`

In upcoming commits, these methods will serve as proxies to
`SpecTxBuilder::get_next_commitment_stats` in all validation of channel
updates in `ChannelContext`.

Eventually, these methods will completely replace
`get_pending_htlc_stats`, and
`get_next_{local, remote}_commit_tx_fee_msat`.

When predicting the HTLCs on next commitment, we take the conservative
approach and only assume that a HTLC will not be in the next commitment
when it is guaranteed that it won't be.

Improve prediction of commitment stats in `can_send_update_fee`

`ChannelContext::get_pending_htlc_stats` predicts that the set of HTLCs
on the next commitment will be all the HTLCs in
`ChannelContext.pending_inbound_htlcs`, and
`ChannelContext.pending_outbound_htlcs`, as well as all the outbound
HTLC adds in the holding cell.

This is an overestimate:

* Outbound HTLC removals which have been ACK'ed by the counterparty will
certainly not be present in any *next* commitment, even though they
remain in `pending_outbound_htlcs` (I refer to states
`AwaitingRemoteRevokeToRemove` and `AwaitingRemovedRemoteRevoke`).

* Outbound HTLCs in the `RemoteRemoved` state, will not be present in
the next *local* commitment.

* Inbound HTLCs in the `LocalRemoved` state will not be present in the
next *remote* commitment.

`ChannelContext::build_commitment_stats(funding, true, true, ..)` makes
these errors when predicting the HTLC count on the remote commitment:

* Inbound HTLCs in the state `RemoteAnnounced` are not included, but
they will be in the next remote commitment transaction if the local
ACK's the addition before producing the next remote commitment.

* Inbound HTLCs in the state `AwaitingRemoteRevokeToAnnounce` are not
included, even though the local has ACK'ed the addition.

* Outbound HTLCs in the state `AwaitingRemoteRevokeToRemove` are
counted, even though the local party has ACK'ed the removal.

This commit replaces these functions in favor of the newly added
`ChannelContext::get_next_{local, remote}_commitment_stats` methods,
and fixes the issues described above.

We now always calculate dust exposure using a buffer from
`msg.feerate_per_kw`, and not from
`max(feerate_per_kw, self.feerate_per_kw, self.pending_update_fee)`.

Improve prediction of commitment stats in `can_accept_incoming_htlc`

`ChannelContext::get_pending_htlc_stats` predicts that the set of HTLCs
on the next commitment will be all the HTLCs in
`ChannelContext.pending_inbound_htlcs`, and
`ChannelContext.pending_outbound_htlcs`, as well as all the outbound
HTLC adds in the holding cell.

This is an overestimate:

* Outbound HTLC removals which have been ACK'ed by the counterparty will
certainly not be present in any *next* commitment, even though they
remain in `pending_outbound_htlcs`.

* Outbound HTLCs in the `RemoteRemoved` state, will not be present in
the next *local* commitment.

* Outbound HTLCs in the `LocalAnnounced` state have no guarantee that
they were yet received by the counterparty.

* Outbound `update_add_htlc`'s in the holding cell are certainly not
known by the counterparty, and we will reevaluate their addition to
the channel when freeing the holding cell.

* Inbound HTLCs in the `LocalRemoved` state will not be present in the
next *remote* commitment.

This commit stops using `get_pending_htlc_stats` in favor of the newly
added `ChannelContext::get_next_{local, remote}_commitment_stats`
methods, and fixes the issues described above.

`ChannelContext::next_remote_commit_tx_fee_msat` counts inbound HTLCs in
the `LocalRemoved` state, as well as outbound HTLCs in the
`LocalAnnounced` state. We now do not count them for the same reasons
described above.

Inbound `LocalRemoved` HTLCs that were **not** successful are now
credited to `remote_balance_before_fee_msat` as they will certainly not
be on the next remote commitment. We previously debited these from the
remote balance to arrive at `remote_balance_before_fee_msat`.

We now always check holder dust exposure, whereas we previously would
only do it if the incoming HTLC was dust on our own commitment
transaction.

Furthermore, dust exposure calculations now take a buffer from the
currently committed feerate, and ignore any fee updates in
`ChannelContext.pending_update_fee`.

Improve prediction of commitment stats in `validate_update_add_htlc`

`ChannelContext::get_pending_htlc_stats` predicts that the set of HTLCs
on the next commitment will be all the HTLCs in
`ChannelContext.pending_inbound_htlcs`, and
`ChannelContext.pending_outbound_htlcs`, as well as all the outbound
HTLC adds in the holding cell.

This is an overestimate:

* Outbound HTLC removals which have been ACK'ed by the counterparty will
certainly not be present in any *next* commitment, even though they
remain in `pending_outbound_htlcs`.

* Outbound HTLCs in the `RemoteRemoved` state, will not be present in
the next *local* commitment.

* Outbound HTLCs in the `LocalAnnounced` state have no guarantee that
they were received by the counterparty before she sent the
`update_fee`.

* Outbound `update_add_htlc`'s in the holding cell are certainly not
known by the counterparty, and we will reevaluate their addition to
the channel when freeing the holding cell.

* Inbound HTLCs in the `LocalRemoved` state will not be present in the
next *remote* commitment.

`ChannelContext::next_local_commit_tx_fee_msat` over-counts outbound
HTLCs in the `LocalAnnounced` and `RemoteRemoved` states, as well as
outbound `update_add_htlc`'s in the holding cell.

`ChannelContext::next_remote_commit_tx_fee_msat` over-counts inbound
HTLCs in the `LocalRemoved` state, as well as outbound HTLCs in the
`LocalAnnounced` state.

This commit stops using these functions in favor of the newly added
`ChannelContext::get_next_{local, remote}_commitment_stats` methods,
and fixes the issues described above.

If we are the funder, we also check that adding this inbound HTLC
doesn't increase the commitment transaction fee to the point of
exhausting our balance on the local commitment. Previously, we would
only subtract the anchors from `funding.value_to_self_msat`; we now
also subtract the outbound HTLCs on the next local commitment from
`funding.value_to_self_msat` before checking if we can afford the
additional transaction fees.

Inbound `LocalRemoved` HTLCs that were **not** successful are now
credited to `remote_balance_before_fee_msat` as they will certainly not
be on the next remote commitment. We previously debited these from the
remote balance to arrive at `remote_balance_before_fee_msat`.

When calculating dust exposure, we now take a buffer from the currently
committed feerate, and ignore any fee updates in
`ChannelContext.pending_update_fee`.

Improve prediction of commitment stats in `validate_update_fee`

`ChannelContext::get_pending_htlc_stats` predicts that the set of HTLCs
on the next commitment will be all the HTLCs in
`ChannelContext.pending_inbound_htlcs`, and
`ChannelContext.pending_outbound_htlcs`, as well as all the outbound
HTLC adds in the holding cell.

This is an overestimate:

* Outbound HTLC removals which have been ACK'ed by the counterparty will
certainly not be present in any *next* commitment, even though they
remain in `pending_outbound_htlcs`.

* Outbound HTLCs in the `RemoteRemoved` state, will not be present in
the next *local* commitment.

* Outbound HTLCs in the `LocalAnnounced` state have no guarantee that
they were received by the counterparty before she sent the
`update_fee`.

* Outbound `update_add_htlc`'s in the holding cell are certainly not
known by the counterparty, and we will reevaluate their addition to
the channel when freeing the holding cell.

* Inbound HTLCs in the `LocalRemoved` state will not be present in the
next *remote* commitment.

This commit stops using `get_pending_htlc_stats` in favor of the newly
added `ChannelContext::get_next_{local, remote}_commitment_stats`
methods, and fixes the issues described above.

We now always calculate dust exposure using a buffer from
`msg.feerate_per_kw`, and not from
`max(self.feerate_per_kw, msg.feerate_per_kw)`.

Add `TxBuilder::get_next_commitment_stats`

Given a snapshot of the lightning state machine,
`TxBuilder::get_next_commitment_stats` calculates the transaction fees,
the dust exposure, and the holder and counterparty balances
(the balances themselves do *not* account for the transaction fee).

Adjust dust exposure due to excess fees for clarity

Remove now-unused ServeStaticInvoice::invoice_id

In the initially-merged version of the static invoice server protocol, the
static invoice server would sometimes have to find a specific static invoice
based on (recipient_id, invoice_slot) and sometimetimes based on (recipient_id,
invoice_id). This made the API harder to use in terms of how the server would
index into the KVStore.

Over the course of the previous commits we transitioned to the server always
finding a specific invoice based on (recipient_id, invoice_slot). We still have
a few dangling references to invoice_id in some messages and events though, so
remove those here.

Replace StaticInvoiceReq::invoice_id with ::inv_slot

In the initially-merged version of the static invoice server protocol, the
static invoice server would sometimes have to find a specific static invoice
based on (recipient_id, invoice_slot) and sometime based on (recipient_id,
invoice_id). This made the API harder to use in terms of how the server would
index into the KVStore.

We'd like to transition to the server always finding a specific invoice based on
(recipient_id, invoice_slot) and get rid of the invoice_id concept.

Previously, when an invoice request would come in for the server on behalf of
the often-offline recipient, they would need to find the static invoice based
on the recipient_id and invoice_id. However, previous commits have now led to
the server being able to use the invoice_slot in the initial offer paths that
they create, which we do here, obviating the need for them to create their own
randomly-generated invoice_id. The final dangling references to the invoice_id
will be removed in the next commit.