The spec mandates that we copy the `sha256_hash_of_onion` field
from the `UpdateFailMalformedHTLC` message into the error message
we send back to the sender, however we simply ignored it. Here we
copy it into the message correctly.
This replaces `final_expiry_too_soon` with
`incorrect_or_unknown_payment` as was done in
https://github.com/lightning/bolts/pull/608. Note that the
rationale for this (that it may expose whether you are the final
recipient for the payment or not) does not currently apply to us -
we don't apply different final CLTV values to different payments.
However, we might in the future, and this will make us slightly
more consistent with other nodes.
Now that `HTLCFailReason` is opaque and in `onion_utils`, we should
encapsulate it so that `ChannelManager` can no longer directly
access its inner fields.
Like the previous commit, here we update the update_fee+commit
logic to simply push the fee update into the holding cell and then
use the standard holding-cell-freeing codepaths to actually send
the commitment update. This removes a substantial amount of code,
reducing redundant codepaths and keeping channel state machine
logic in channel.rs.
We currently free the channel holding cells in
`get_and_clear_pending_msg_events`, blocking outbound messages
while we do so. This is fine, but may block the message pipeline
longer than we need to. In the next commit we'll push
timer-originating channel fee updates out through the holding cell
pipeline, leaning more on that freeing in the future.
Thus, to avoid a regression in message time, here we clear the
holding cell after processing all timer events. This also avoids
needing to change tests in the next commit.
When we batch HTLC updates, we currently do the explicit queueing
plus the commitment generation in the `ChannelManager`. This is a
bit strange as its ultimately really a `Channel` responsibility to
generate commitments at the correct time, with the abstraction
leaking into `ChannelManager` with the `send_htlc` and
`get_update_fail_htlc` method docs having clear comments about
how `send_commitment` MUST be called prior to calling other
`Channel` methods.
Luckily `Channel` already has an update queue - the holding cell.
Thus, we can trivially rewrite the batch update logic as inserting
the desired updates into the holding cell and then asking all
channels to clear their holding cells.
When a channel is force-closed, if a `ChannelMonitor` update is
completed but a `ChannelManager` persist has not yet happened,
HTLCs which were removed in the latest (persisted) `ChannelMonitor`
update will not be failed even though they do not appear in the
commitment transaction which went on chain. This is because the
`ChannelManager` thinks the `ChannelMonitor` is responsible for
them (as it is stale), but the `ChannelMonitor` has no knowledge of
the HTLC at all (as it is not stale).
The fix for this is relatively simple - we need to check for this
specific case and fail back such HTLCs when deserializing a
`ChannelManager`
If, after forwarding a payment to our counterparty, we restart with
a ChannelMonitor update having been persisted, but the
corresponding ChannelManager update was not persisted, we'll still
have the forwarded HTLC in the `forward_htlcs` map on start. This
will cause us to generate a (spurious) `PendingHTLCsForwardable`
event. However, when we go to forward said HTLC, we'll notice the
channel has been closed and leave it up to the `ChannelMontior` to
finalize the HTLC.
This is all fine today - we won't lose any funds, we'll just
generate an excess forwardable event and then fail to forward.
However, in the future when we allow for forward-time channel
changes this could break. Thus, its worth adding tests for this
behavior today, and, while we're at it, removing the spurious
forwardable HTLCs event.
This expands the outbound-HTLC-listing support in `ChannelMonitor`
to include not only the set of outbound HTLCs which have not yet
been resolved but to also include the full set of HTLCs which the
`ChannelMonitor` is currently able to to or has already finalized.
This will be used in the next commit to fail-back HTLCs which were
removed from a channel in the ChannelMonitor but not in a Channel.
Using the existing `get_pending_outbound_htlcs` for this purpose is
subtly broken - if the channel is already closed, an HTLC fail may
have completed on chain and is no longer "pending" to the monitor,
but the fail event is still in the monitor waiting to be handed
back to the `ChannelMonitor` when polled.
Since `ChannelMonitor`s will now re-derive signers rather than
persisting them, we can no longer use the OnlyReadsKeysInterface
concrete implementation.
Similar to the previous commit, we introduce a new serialization version
that doesn't store a monitor's signer. Since the monitor already knows
of a channel's `channel_keys_id`, there's no need to store any new data
to re-derive all private key material for said channel.
To do so, we introduce a new serialization version that doesn't store a
channel's signer, and instead stores its signer's `channel_keys_id`.
This is a unique identifier that can be provided to our `KeysInterface`
to re-derive all private key material for said channel.
We choose to not upgrade the minimum compatible serialization version
until a later time, which will also remove any signer serialization
logic on implementations of `KeysInterface` and `Sign`.
Now that ready_channel is also called on startup upon deserializing
channels, we opt to rename it to a more indicative name.
We also derive `PartialEq` on ChannelTransactionParameters to allow
implementations to determine whether `provide_channel_parameters` calls
are idempotent after the channel parameters have already been provided.
`get_channel_signer` previously had two different responsibilites:
generating unique `channel_keys_id` and using said ID to derive channel
keys. We decide to split it into two methods `generate_channel_keys_id`
and `derive_channel_signer`, such that we can use the latter to fulfill
our goal of re-deriving signers instead of persisting them. There's no
point in storing data that can be easily re-derived.
Soon we're going to need to return an error when ChannelManager is unable to
find a route, so we'll need a way to distinguish between that and the user
supplying an invalid route.
Currently we loop over `htlcs_to_fail` locking `channel_state` for each
element only to call `get_htlc_inbound_temp_fail_err_and_data` with the
same inputs on each iteration. This is unnecessary, we can refactor and
call `get_htlc_inbound_temp_fail_err_and_data` outside of the loop.
Currently `fail_htlc_backwards_internal` takes ownership of its source
and reason parameters however they are not consumed so we can borrow them.
Includes refactoring to use local variables before the function call.
We create `HTLCFailReason` inline in function calls in a bunch of places
in the `channelmanager` module, we can make the code more terse with no
loss of clarity by implementing a couple of constructor methods.
The `derive_{public,private}_revocation_key` methods hash the two
input keys and then multiply the two input keys by hashed values
before adding them together. Because addition can fail if the tweak
is the inverse of the secret key this method currently returns a
`Result`.
However, it is not cryptographically possible to reach the error
case - in order to create an issue, the point-multiplied-by-hash
values must be the inverse of each other, however each point
commits the SHA-256 hash of both keys together. Thus, because
changing either key changes the hashes (and the ultimate points
added together) in an unpredictable way, there should be no way to
construct such points.
The `derive_{public,private}_key` methods hash the two input keys
and then add them to the input public key. Because addition can
fail if the tweak is the inverse of the secret key this method
currently returns a `Result`.
However, it is not cryptographically possible to reach the error
case - in order to create an issue, the SHA-256 hash of the
`base_point` (and other data) must be the inverse of the
`base_point`('s secret key). Because changing the `base_point`
changes the hash in an unpredictable way, there should be no way to
construct such a `base_point`.
