`sign_holder_commitment_and_htlcs` never really made sense. Unlike
`sign_counterparty_commitment`, the signatures for holder HTLC
transactions may be required much later than the commitment
transaction's. While it was nice for us to only reach the signer once to
obtain all holder signatures, it's not really ideal anymore as we want
our signatures to be random and not reused.
We no longer return all holder HTLC signatures and instead defer to
obtaining them via `EcdsaChannelSigner::sign_holder_htlc_transaction`.
We want to ensure we use fresh random signatures to prevent certain
classes of transaction replacement attacks at the bitcoin P2P layer.
This was already covered for commitment transactions and zero fee holder
HTLC transactions, but was missing for holder HTLC transactions on
non-anchors channels.
We can easily do this by reusing the existing
`EcdsaChannelSigner::sign_holder_htlc_transaction` method and
circumventing the existing `holder_htlc_sigs/prev_holder_htlc_sigs`
caches, which will be removed in a later commit anyway.
While our commitment transactions did use the correct `to_remote`
script, the `ChannelMonitor`'s was not as it is tracked separately. This
would lead to users never receiving an `Event::SpendableOutputs` with a
`StaticPaymentOutput` descriptor to claim the funds.
Luckily, any users affected which had channel closures confirmed by a
counterparty commitment just need to replay the closing transaction to
receive the event.
Assuming our keys haven't been compromised, and that random transactions
aren't learning of these scripts somehow and sending funds to them, it
was only possible for one spendable output to exist within a
transaction.
- `shutdown_script` can only exist in co-op close transactions.
- `counterparty_payment_script` can only exist in counterparty
commitment transactions.
- `broadcasted_holder_revokable_script` can only exist in holder
commitment/HTLC transactions.
- `destination_script` can exist in any other type of claim we support.
Now that we're exposing this API to users such that they can rescan any
relevant transactions, there's no harm in allowing them to claim more
funds from spendable outputs than we expected.
Currently, our API will only expose `SpendableOutputDescriptor`s once
after they are no longer under reorg risk (see `ANTI_REORG_DELAY`).
Users have often requested they'd like the ability to retrieve these in
some other way, either for historical purposes, or to handle replaying
any in the event of a failure.
HTLC outputs, like the `to_remote` output, in commitment transactions
with anchor outputs also have an additional `1 CSV` constraint on the
counterparty. When spending such outputs, their corresponding input
needs to have their sequence set to 1. This was done for HTLC claims
from holder commitments, but unfortunately not for counterparty
commitments as we were lacking test coverage.
The `MonitorEvent::CommitmentTxConfirmed` has always been a result
of us force-closing the channel, not the counterparty doing so.
Thus, it was always a bit of a misnomer. Worse, it carried over
into the channel's `ClosureReason` in the event API.
Here we simply rename it and use the proper `ClosureReason`.
Releasing write locks in between monitor updates
requires storing a set of cloned keys to iterate
over. For efficiency purposes, that set of keys
is an actual set, as opposed to array, which means
that the iteration order may not be consistent.
The test was relying on an event array index to
access the revocation transaction. We change that
to accessing a hash map keyed by the txid, fixing
the test.
This will make it possible to
link between SpendableOuts and ChannelMonitor
- change channel_id to option so we dont break upgrade
- remove unused channel_id
- document channel_id
- extract channel id dynamically to pass test
- use contains to check channel_id in test as the events are not ordered
- update docs framing
- specify ldk version channel_id will be introduced in
Co-authored-by: Elias Rohrer <dev@tnull.de>
Update lightning/src/events/mod.rs
Co-authored-by: Elias Rohrer <dev@tnull.de>
In 0ad1f4c943 we fixed a nasty bug
where a failure to persist a `ChannelManager` faster than a
`ChannelMonitor` could result in the loss of a `PaymentSent` event,
eventually resulting in a `PaymentFailed` instead!
As noted in that commit, there's still some risk, though its been
substantially reduced - if we receive an `update_fulfill_htlc`
message for an outbound payment, and persist the initial removal
`ChannelMonitorUpdate`, then respond with our own
`commitment_signed` + `revoke_and_ack`, followed by receiving our
peer's final `revoke_and_ack`, and then persist the
`ChannelMonitorUpdate` generated from that, all prior to completing
a `ChannelManager` persistence, we'll still forget the HTLC and
eventually trigger a `PaymentFailed` rather than the correct
`PaymentSent`.
Here we fully fix the issue by delaying the final
`ChannelMonitorUpdate` persistence until the `PaymentSent` event
has been processed and document the fact that a spurious
`PaymentFailed` event can still be generated for a sent payment.
The original fix in 0ad1f4c943 is
still incredibly useful here, allowing us to avoid blocking the
first `ChannelMonitorUpdate` until the event processing completes,
as this would cause us to add event-processing delay in our general
commitment update latency. Instead, we ultimately race the user
handling the `PaymentSent` event with how long it takes our
`revoke_and_ack` + `commitment_signed` to make it to our
counterparty and receive the response `revoke_and_ack`. This should
give the user plenty of time to handle the event before we need to
make progress.
Sadly, because we change our `ChannelMonitorUpdate` semantics, this
change requires a number of test changes, avoiding checking for a
post-RAA `ChannelMonitorUpdate` until after we process a
`PaymentSent` event. Note that this does not apply to payments we
learned the preimage for on-chain - ensuring `PaymentSent` events
from such resolutions will be addressed in a future PR. Thus, tests
which resolve payments on-chain switch to a direct call to the
`expect_payment_sent` function with the claim-expected flag unset.
When reloading a node in the test framework, we end up with a new
`ChannelManager` that has references to various test util structs.
In order for the tests to compile reliably in the face of unrelated
changes, those test structs need to always be initialized before
both the new but also the original `ChannelManager`.
Here we make that change.
In Java/TypeScript, we map enums as a base class and each variant
as a class which extends the base. In Java/TypeScript, functions
and fields share the same namespace, which means we cannot have
functions on an enum which have the same name as any fields in any
enum variants.
`Balance`'s `claimable_amount_satoshis` method aliases with fields
in each variant, and thus ultimately doesn't compile in TypeScript.
Because `Balance::claimable_amount_satoshis` has the same name as
the fields, it's also a bit confusing, as it doesn't return the
field for each variant, but sometimes returns zero if we're not
sure we can claim the balance.
Instead, we rename the fields in each enum variant to simply
`amount_satoshis`, to avoid implying that we can definitely claim
the balance.
There's no need to yield such an event when the commitment transaction
already meets the target feerate on its own, so we can simply broadcast
it without an anchor child transaction. This may be a common occurrence
until we are less aggressive about feerate updates.
Since the use of channels with anchor outputs requires a reserve of
onchain funds to handle channel force closures, it would be
irresponsible to allow a node to accept inbound channel without first
consulting such reserves. To allow users to do so, we require such
channels be manually accepted.
Now that all of the core functionality for anchor outputs has landed,
we're ready to remove the config flag that was temporarily hiding it
from our API.
This change modifies six structs that were keeping
track of anchors features with an `opt_anchors` field,
as well as another field keeping track of nonzero-fee-
anchor-support.
The purpose of this payload is to ensure we retry restored packages on a
`ChannelMonitor` that has upgraded from a version that previously did
not have such retry logic. We can verify this works by checking whether
a restored package has a `height_timer` of `None` upon deserializing the
monitor payload.
In the previous commit, we added a helper that constructs blocks
whenever tests demand blocks be connected. This helper moved towards
having all connected blocks have a version of 0x2000_0000 (also known as
NO_SOFT_FORK_SIGNALLING). However, previously, it was possible for some
blocks to be connected with a slighty different version: 0x0200_0000,
resulting in different block hashes.
This block hash divergence prompted a failure in this test when
`ConnectStyle::HighlyRedundantTransactionsFirstSkippingBlocks` is used
for `nodes[0]`, since this block connection style reconfirms
transactions redundantly and the serialized monitor payload kept a
reference to the hash of the block with version 0x0200_0000, when it
should be expecting one with version 0x2000_0000.
