Matt Corallo [Thu, 5 Oct 2023 23:57:08 +0000 (23:57 +0000)]
Make clippy shut up about `PartialOrd` and `Ord` both impl'd
Clippy gets mad that we have an implementation of `ParialOrd` and
`Ord` separately, even though both are identical. Making
`ParitalOrd` call `Ord` makes clippy shut up.
Matt Corallo [Tue, 3 Oct 2023 04:12:28 +0000 (04:12 +0000)]
Make `InMemorySigner` parameter-fetching utilities return `Option`s
In the previous commit we fixed a bug where we were spuriously
(indirectly) `unwrap`ing the `channel_parameters` in
`InMemorySigner`. Here we make such bugs much less likely in the
future by having the utilities which do the `unwrap`ing internally
return `Option`s instead.
Matt Corallo [Sun, 1 Oct 2023 00:15:22 +0000 (00:15 +0000)]
Avoid unwrap'ing `channel_parameters` in to_counterparty signing
Previously, `StaticPaymentOutputDescriptor`s did not include
`channel_parameters` for the signer. As a result, when going to
spend old `StaticPaymentOutputDescriptor`s,
`InMemorySigner::sign_counterparty_payment_input` may be called
with `channel_parameters` set to `None`. This should be fine, but
in fa2a2efef47b5da48ac7a239f3d8a835a7f28164 we started relying on
it (indirectly via `channel_features`) for signing. This caused an
`unwrap` when spending old output descriptors.
This is fixed here by simply avoiding the unwrap and assuming old
`StaticPaymentOutputDescriptor`s represent non-anchor channels.
Matt Corallo [Thu, 28 Sep 2023 02:45:00 +0000 (02:45 +0000)]
Use `crate::prelude::*` to load `Vec`, rather than `crate::Vec`
This is kinda dumb, but the bindings get confused when referring
to `Vec` absolutely in a `use` statement, and there's no reason not
to load our prelude everywhere.
Matt Corallo [Wed, 27 Sep 2023 23:02:50 +0000 (23:02 +0000)]
Drop various bounds on types passed to `MonitorUpdatingPersister`
The new `MonitorUpdatingPersister` has a few redundant type bounds
(re-specified on functions after having been specified on the
struct itself), which we remove here.
Further, it requires a `Deref<FeeEstimator>` which is `Clone`able.
This is generally fine in rust, but annoying in bindings, so we
simply elide it in favor if a `&Deref<FeeEstimator>`.
Matt Corallo [Mon, 25 Sep 2023 22:10:27 +0000 (22:10 +0000)]
Make `create_onion_message` a freestanding function
The new `create_onion_message` function in `OnionMessenger` is hard
to handle - it has various generic requirements indirectly via the
struct, but they're not bounded by any of the method parameters.
Thus, you can't simply call `OnionMessenger::create_onion_message`,
as various bounds are not specified.
Instead, we move it to a freestanding function so that it can be
called directly without explicitly setting bounds.
Matt Corallo [Mon, 25 Sep 2023 22:14:04 +0000 (22:14 +0000)]
Mark `AChannelManager` no-export
The trait itself has no purpose for bindings, as all structs are
concretized anyway. Further, the bindings have specific handling
for generic bounding traits like this.
Matt Corallo [Sun, 24 Sep 2023 23:41:10 +0000 (23:41 +0000)]
Mark `SpendableOutputDescriptor::to_psbt_input` as no-export
Its honestly likely not all that useful as its not materially
interoperable with other PSBT libraries. Instead, users should
simply fetch the full PSBT and use the inputs from it as they see
fit.
Matt Corallo [Sun, 24 Sep 2023 23:59:13 +0000 (23:59 +0000)]
Simplify score bounding with a unified type
In a few places we require a unified scorer, which implements both
`ScoreLookUp` and `ScoreUpdate`. Rather than double-bounding (which
the bindings generator can't handle directly), we use a top-level
`Score` trait which requires both and is implemented for all
implementers of both supertraits.
Matt Corallo [Thu, 21 Sep 2023 22:54:14 +0000 (22:54 +0000)]
Remove unnecessary bounds in scoring
In our scoring logic we have a handful of unnecessary bounds,
leading to extra diff in the bindings branch when those bounds have
to be removed as well as a few cases where bindings generation
simply gets confused.
Here we remove a number of bounds across the scoring traits and
impls, cleaning things up and simplifying bindings changes.
Matt Corallo [Thu, 21 Sep 2023 22:22:18 +0000 (22:22 +0000)]
Use `Default::default()` to construct `()` as score-updating param
In 6b0d94a3029f74de3a7542cbba0d48c2f7e5866b we switched most tests
to `Default::default()` for scoring parameters passed to
route-fetching. Here we do the same for the scoring parameters when
passed to score-updating.
Matt Corallo [Sat, 30 Sep 2023 18:05:10 +0000 (18:05 +0000)]
Expose `parse_onion_address` publicly in `no-std`
The reason for having a separate `parse_onion_address` from
`FromStr` is to have an onion parsing function in `no-std`, but
when we added it we forgot to make it public. We do this here, as
well as fix a few compilation warnings in `no-std`.
Matt Corallo [Sat, 30 Sep 2023 17:32:28 +0000 (17:32 +0000)]
Fix `Simple*PeerManager` to not require refs to the `UtxoLookup`
`UtxoLookup` doesn't strictly need to be referenced from the
`PeerManager`, and in fact the new `GossipVerifier` in
`lightning-block-sync` requires itself to be owned by the
`PeerManager` (for circular type reasons).
This allows us to use `lightning-block-sync`'s `GossipVerifier`
with `SimpleArcPeerManager` in ldk-sample.
Matt Corallo [Sat, 30 Sep 2023 17:35:21 +0000 (17:35 +0000)]
Implement `Readable`/`Writeable` for `RwLock` wrappers
We now support separate R/W locks in `LockableScore`, which allow
us to do routefinding in parallel, however in order to support
`WriteableScore` for such users we need to implement `Writeable`
for `RwLock` wrappers around `Writeable` types, which we do here.
Persist full monitor if there is an error while applying monitor_update
Motivation: When there is an error while applying monitor_update to a
channel_monitor, we don't want to persist a 'monitor_update' which
results in a failure to apply later while reading 'channel_monitor' with
updates from storage. Instead, we should persist the entire 'channel_monitor'
here.
We were incorrectly marking updates as chain_sync
or not in test_utils based on whether monitor_update
is None or not. Instead, use UpdateOrigin to determine it.
Matt Corallo [Sun, 24 Sep 2023 02:32:08 +0000 (02:32 +0000)]
Fix matching of second-stage HTLC claim in get_htlc_balance
We incorrectly assumed that the descriptor's output index from
second-stage HTLC transaction would always match the HTLC's output index
in the commitment transaction. This doesn't make any sense though, we
need to make sure we map the descriptor to it's corresponding HTLC in
the commitment. Instead, we check that the transaction from which the
descriptor originated from spends the HTLC in question.
Note that pre-anchors, second-stage HTLC transactions are always 1
input-1 output, so previously we would only match if the HTLC was the
first output in the commitment transaction. Post-anchors, they are
malleable, so we can aggregate multiple HTLC claims into a single
transaction making this even more likely to happen. Unfortunately, we
lacked proper coverage in this area so the bug went unnoticed. To
address this, we aim to extend our existing coverage of
`get_claimable_balances` to anchor outputs channels in the following
commits.
Support signing to_remote anchors variant for StaticPaymentOutput
`to_remote` outputs on commitment transactions with anchor outputs have
an additional `1 CSV` constraint on its spending condition,
transitioning away from the previous P2WPKH script to a P2WSH.
Since our `ChannelMonitor` was never updated to track the proper
`to_remote` script on anchor outputs channels, we also missed updating
our signer to handle the new script changes.
Use correct to_remote script in counterparty commitments
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.
Avoid early return upon confirmation of channel funding
This early return is only possible if the channel requires a single
confirmation, allowing a `channel_ready` message to go out. This can be
problematic though if a commitment transaction (specifically from the
counterparty, as the channel would be immediately closed if a local
commitment is broadcast) also confirms within the same block. The
`ChannelMonitor` will detect both, but it won't inform the
`ChannelManager` at all. Luckily, while the channel still is considered
open to the `ChannelManager`, the `ChannelMonitor` will reject any
further updates to the channel state.
Matt Corallo [Fri, 29 Sep 2023 18:32:25 +0000 (18:32 +0000)]
Revert "Remove AvailableBalances::balance_msat"
While removing the `balance_msat` field absolutely makes sense -
it is, at best, confusing - we really need a solid replacement for
it before we can do so. While one such replacement is in progress,
it is not complete and we'd like to not block our current release
on its completion.
Wilmer Paulino [Mon, 14 Aug 2023 19:12:54 +0000 (12:12 -0700)]
Test preimage claim after reorg of counterparty commitment
This test adds coverage for receiving a preimage after seeing a
counterparty commitment confirm, followed by a reorg and the
confirmation of a different commitment instead.
The first test covers the case where a holder commitment confirms after
the counterparty commitment reorg.
The second test covers the case where a previous counterparty commitment
confirms after the latest counterparty commitment reorg.
Claim HTLCs with preimage from currently confirmed commitment
We should always claim HTLCs from the currently confirmed commitment,
rather than always claiming from the latest or previous counterparty
commitment if we've seen either confirm onchain at a prior point.
Avoid persisting on same counterparty's ChannelUpdate
Some nodes may rebroadcast their `ChannelUpdate` to their counterparty
on every connection establishment, which leads to us doing an additional
persist most of the time when nothing has changed. Now, we'll only
persist if we receive an update that changes anything.
Retrieve all possible spendable outputs from transactions
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.
Allow retrieval of SpendableOutputDescriptors from relevant transactions
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.
Matt Corallo [Thu, 28 Sep 2023 18:19:36 +0000 (18:19 +0000)]
Include any recipient overpayment amounts in the route fee limit
If the user told us to limit their total fee exposure, we should
do so including any potential overpayment to the recipient, which
is ultimately a part of the "fee" as far as the user is concerned.
Matt Corallo [Tue, 26 Sep 2023 21:58:53 +0000 (21:58 +0000)]
Try to overpay the recipient if we fail to find a path at all
Previously we'd only try to overpay if we managed to find a path
to the recipient which was sufficient. However, if we fail to find
any path to the recipient at all we should still retry overpaying
the recipient. Ultimately we should be silling to pay whatever
reasonable performance penalty if the alternative is not finding a
path at all, which we do here.
Matt Corallo [Thu, 28 Sep 2023 18:19:50 +0000 (18:19 +0000)]
Allow routefinding again due to HTLC limit based on >= not >
While this doesn't matter much in practice, if we go around again
when route-finding to try to meet an htlc_minimum_msat, we use the
`recommended_value_msat` which can work if we meet the
`htlc_minimum_msat` on a channel exactly, so using >= rather than >
can capture cases with 1msat more.
Elias Rohrer [Fri, 22 Sep 2023 14:38:30 +0000 (16:38 +0200)]
Avoid adding duplicate hint candidates if they are first hops
If we have a direct channel to a node generating an invoice with route
hints, we'd previously happily add multiple candidates that all refer to
the same channel. To keep our candidate set small and unify our tracking
where possible, we now check if its `short_channel_id` is an
`outbound_scid_alias` of any of our first hops and refrain from adding
another candidate if it's the case.
Elias Rohrer [Fri, 22 Sep 2023 13:56:07 +0000 (15:56 +0200)]
Also add route hints if we are the source
Previously, we would only consider route hints if we had a direct
channel to the first node in the hint or if the first node in the hint
was part of the public network graph.
However, this left out the possiblity of us being part of the first hop,
especially if our own node is not announced and part of the graph.