Previously, `Confirm::get_relevant_txids()` only returned a list of
transactions that have to be monitored for reorganization out of the
chain. This interface however required double bookkeeping: while we
internally keep track of the best block, height, etc, it would also
require the user to keep track which transaction was previously
confirmed in which block and to take actions based on any change, e.g,
to reconfirm them when the block would be reorged-out and the
transactions had been reconfirmed in another block.
Here, we track the confirmation block hash internally and return it via
`Confirm::get_relevant_txids()` to the user, which alleviates the
requirement for double bookkeeping: the user can now simply check
whether the given transaction is still confirmed and in the given block,
and take action if not.
We also split `update_claims_view`: Previously it was one, now it's two
methods: `update_claims_view_from_matched_txn` and
`update_claims_view_from_requests`.
These claims will never be valid as a previous claim has already
confirmed. If a previous claim is reorged out of the chain, a new claim
will be generated bypassing the new behavior.
While this doesn't change much for our existing transaction-based
claims, as broadcasting an already confirmed transaction acts as a NOP,
it prevents us from yielding redundant event-based claims, which will be
introduced as part of the anchors patchset.
In c986e52ce8, an `MppId` was added
to `HTLCSource` objects as a way of correlating HTLCs which belong
to the same payment when the `ChannelManager` sees an HTLC
succeed/fail. This allows it to have awareness of the state of all
HTLCs in a payment when it generates the ultimate user-facing
payment success/failure events. This was used in the same PR to
avoid generating duplicative success/failure events for a single
payment.
Because the field was only used as an internal token to correlate
HTLCs, and retries were not supported, it was generated randomly by
calling the `KeysInterface`'s 32-byte random-fetching function.
This also provided a backwards-compatibility story as the existing
HTLC randomization key was re-used for older clients.
In 28eea12bbe `MppId` was renamed to
the current `PaymentId` which was then used expose the
`retry_payment` interface, allowing users to send new HTLCs which
are considered a part of an existing payment.
At no point has the payment-sending API seriously considered
idempotency, a major drawback which leaves the API unsafe in most
deployments. Luckily, there is a simple solution - because the
`PaymentId` must be unique, and because payment information for a
given payment is held for several blocks after a payment
completes/fails, it represents an obvious idempotency token.
Here we simply require the user provide the `PaymentId` directly in
`send_payment`, allowing them to use whatever token they may
already have for a payment's idempotency token.
Useful since we're working on getting rid of KeysInterface::get_node_secret to
complete support for remote signing.
Will be used in upcoming work to check whether an outbound onion message
blinded path has our node id as the introduction node id
As we integrate the support of anchor outputs, we'll want to know if
each input we're working with came from an anchor outputs channel.
Instead of threading through a `opt_anchors` boolean across several
methods on `PackageSolvingData` and `PackageTemplate`, we decide to
store a reference in each `PackageSolvingData` variant instead that
features a change in behavior between channels with and without anchor
outputs.
When a `chain::Watch` `ChannelMonitor` update method is called, the
user has three options:
(a) persist the monitor update immediately and return success,
(b) fail to persist the monitor update immediately and return
failure,
(c) return a flag indicating the monitor update is in progress and
will complete in the future.
(c) is rather harmless, and in some deployments should be expected
to be the return value for all monitor update calls, but currently
requires returning `Err(ChannelMonitorUpdateErr::TemporaryFailure)`
which isn't very descriptive and sounds scarier than it is.
Instead, here, we change the return type used to be a single enum
(rather than a Result) and rename `TemporaryFailure`
`UpdateInProgress`.
See doc updates for more info on the edge case this prevents, and
there isn't really a strong reason why we would need to broadcast
the latest state immediately. Specifically, in the case of HTLC
claims (the most important reason to ensure we have state on chain
if it cannot be persisted), we will still force-close if there are
HTLCs which need claiming and are going to expire.
Surprisingly, there were no tests which failed as a result of this
change, but a new one has been added.
As we move towards specify supported/required feature bits in the
module(s) where they are supported, the global `known` feature set
constructors no longer make sense.
In anticipation of removing the `known` constructor, this commit
removes all remaining references to it outside of features.rs.
HTLC transactions from anchor channels are constrained by a CSV of 1
block, so broadcasting them along with the unconfirmed commitment
tranasction will result in them being immediately rejected as premature.
There's no need to broadcast our local commitment transaction if we've
already seen a confirmed one as it'll be immediately rejected as a
duplicate/conflict.
This will also help prevent dispatching spurious events for bumping
commitment and HTLC transactions through anchor outputs (once
implemented in future work) and the dispatch for said events follows the
same flow as our usual commitment broadcast.
5a8ede09fb updated the documentation
on `get_claimable_balance` to note that if a channel went on-chain
with an LDK version older than 0.0.108 some
counterparty-revoked-output claimable balances my be missing.
However, this failed to account for the fact that we rely on the
entirely-new-in-0.0.111
`confirmed_commitment_tx_counterparty_output` field for some
balances as well.
Thus, the comment should have been in terms of 0.0.111, not
0.0.108.
Previously, we wouldn't mark a dust HTLC as permanently resolved if
the commitment transaction went on chain. This resulted in us
always considering the HTLC as pending on restart, when we load the
pending payments set from the monitors.
Fixes#1653.
If we don't currently have the preimage for an inbound HTLC, that
does not guarantee we can never claim it, but instead only that we
cannot claim it unless we receive the preimage from the channel we
forwarded the channel out on.
Thus, we cannot consider a channel to have no claimable balances if
the only remaining output on the commitment ransaction is an
inbound HTLC for which we do not have the preimage, as we may be
able to claim it in the future.
This commit addresses this issue by adding a new `Balance` variant
- `MaybePreimageClaimableHTLCAwaitingTimeout`, which is generated
until the HTLC output is spent.
Fixes#1620
Users need to make decisions about storage sizing and we need to
have advice on the maximum size of various things users need to
store. ChannelMonitorUpdates are likely the worst case of this,
they're usually at max a few KB, but can get up to a few hundred
KB for commitment transactions that have 400+ HTLCs pending.
We had one user report an update (likely) going over 400 KiB, which
isn't immediately obvious to me is practical, but its within a few
multiples of trivially-reachable sizes, so its likely that did
occur. To be on the safe side, we simply recommend users ensure
they can support "upwards of 1 MiB" here.
Val suggested this as an obvious cleanup to separate per_HTLC logic
from the total commitment transaction logic, separating the large
function into two.
This uses the various new tracking added in the prior commits to
expose a new `Balance` type - `CounterpartyRevokedOutputClaimable`.
Some nontrivial work is required, however, as we now have to track
HTLC outputs as spendable in a transaction that comes *after* an
HTLC-Success/HTLC-Timeout transaction, which we previously didn't
need to do. Thus, we have to check if an
`onchain_events_awaiting_threshold_conf` event spends a commitment
transaction's HTLC output while walking events. Further, because
we now need to track HTLC outputs after the
HTLC-Success/HTLC-Timeout confirms, and because we have to track
the counterparty's `to_self` output as a contentious output which
could be claimed by either party, we have to examine the
`OnchainTxHandler`'s set of outputs to spend when determining if
certain outputs are still spendable.
Two new tests are added which test various different transaction
formats, and hopefully provide good test coverage of the various
revoked output paths.
Instead of a series of different
`onchain_events_awaiting_threshold_conf.iter()...` calls to scan
for HTLC status in balance calculation, pull them all out into one
`for ... { match ... }` to do it once and simplify the code
somewhat.
We need this information when we look up if we still need to spend
a revoked output from an HTLC-Success/HTLC-Timeout transaction for
balance calculation.
When a counterparty broadcasts a revoked commitment transaction,
followed immediately by HTLC-Success/-Timeout spends thereof, we'd
like to have an `onchain_events_awaiting_threshold_conf` entry
for them.
This does so using the `HTLCSpendConfirmation` entry, giving it
(slightly) new meaning. Because all existing uses of
`HTLCSpendConfirmation` already check if the relevant commitment
transaction is revoked first, this should be trivially backwards
compatible.
We will ultimately figure out if something is being spent via the
`OnchainTxHandler`, but to do so we need to look up the output via
the HTLC transaction txid, which this allows us to do.
