Matt Corallo [Tue, 3 Sep 2024 15:09:32 +0000 (15:09 +0000)]
Correct `ANCHOR_INPUT_WITNESS_WEIGHT` constant
`ANCHOR_INPUT_WITNESS_WEIGHT` is too high by two weight units,
likely it was calculated to include the SegWit marker bytes, but
it is used to describe an `Input::satisfaction_weight`, which does
not expect the marker bytes.
This corrects that oversight, reducing the constant by two and
adding the marker bytes back in our own internal weight
calculations. It also fixes a second issue where the constant was
too low by one when `grind_signatures` is not set, as that may
result in a signature being one byte longer than we expect.
Matt Corallo [Thu, 29 Aug 2024 22:47:32 +0000 (22:47 +0000)]
Drop bogus debug assertion that we don't overpay on fees
We should always select at least as many coins as is required to
meet the feerate target, but its perfectly fine if we overshoot.
Specifically, we may overshoot deliberately if we choose to burn
change to fee instead.
Matt Corallo [Thu, 29 Aug 2024 20:43:10 +0000 (20:43 +0000)]
Handle under-coin-selecting due to an added OP_RETURN output
When we do coin selection for channel close anchor spends, we may
do coin selection targeting exactly the input values we need.
However, if coin selection does not include a change output, we may
add an OP_RETURN output, which may cause us to end up with less
fee than we wanted on the resulting package.
Here we address this issue by running coin selection twice - first
without seeking the extra weight of the OP_RETURN output, and again
if we find that we under-selected.
Matt Corallo [Thu, 29 Aug 2024 16:45:27 +0000 (16:45 +0000)]
Use `u64` for `required_unknown_bits_from` indexes, not `usize`
While `usize` should be fine, we're multiplying the index by 8 so
if we have a jumbo feature bit fitting in a 32-bit size type may
not quite work. More importantly, this would be the first use of a
`usize` in the public API and dealing with it in bindings is
annoying so we just replace with a `u64`.
Arik Sosman [Wed, 28 Aug 2024 16:33:14 +0000 (09:33 -0700)]
Process updates before archiving monitors.
Previously, `MonitorUpdatingPersister` was disregarding any unapplied
monitor updates when archiving them. This commit ensures that upon
reading monitors, their corresponding updates are also read and
applied prior to archiving.
Arik Sosman [Wed, 28 Aug 2024 16:32:35 +0000 (09:32 -0700)]
Store Broadcaster and FeeEstimator on MonitorUpdatingPersister.
`MonitorUpdatingPersister` does not currently correctly archive
monitors because it neglects any unapplied updates. In order to start
applying these updates, the archiving methods will require access to
instances of `BroadcasterInterface` and `FeeEstimator`.
This commit requires that the `MonitorUpdatingPersister` be
instantiated with those instances, obviating the need for passing
them around, and laying the foundation for the following commit.
Matt Corallo [Wed, 28 Aug 2024 14:35:54 +0000 (14:35 +0000)]
Correct manual shutdown detection on channel closure
In 5e874c3dc9cf1606a3cbbccab3a0d25089a97280 we'd intended to not
reveal the dummy funding transaction in `Event::DiscardFunding`.
However, instead of looking at the channel that was just closed,
the logic only looks at any other channels which were funded as a
part of the same batch. Because manually-funded transactions
cannot currently be done for batch funding, this was actually dead
code, preventing the new changes from taking effect.
Matt Corallo [Tue, 27 Aug 2024 16:47:57 +0000 (16:47 +0000)]
Test new `ConfirmationTarget` selection based on HTLC set
This updates `test_yield_anchors_events` to test both anchor
channels with and without HTLCs, and relies on overriding only the
singular expected `ConfirmationTarget` used, testing the new
`ConfirmationTarget::UrgentOnChainSweep` use.
Matt Corallo [Tue, 27 Aug 2024 16:44:58 +0000 (16:44 +0000)]
Don't ignore events in `test_yield_anchors_events`
Our tests should never ignore the events generated as they provide
critical context about what's happening in LDK. Here we fix
`test_yield_anchors_events` to avoid doing so.
Matt Corallo [Tue, 27 Aug 2024 19:17:06 +0000 (19:17 +0000)]
Only generate an `Event::DiscardFunding` when we need to
5e874c3dc9cf1606a3cbbccab3a0d25089a97280 changed
`Event::DiscardFunding` to not include a dummy transaction when we
were funded without a full funding tx, but in doing so started
generating `DiscardFunding` events on every channel closure rather
than only when there's actually still a pending funding broadcast.
This restores the previous behavior to only generate the event when
we should actually discard the funding tx.
Vincenzo Palazzo [Tue, 27 Aug 2024 08:18:28 +0000 (10:18 +0200)]
event: store the outpoint when is_manual_broadcast
With [1], it's possible to specify `manual_broadcast` for
the channel funding transaction. When `is_manual_broadcast` is
set to true, the transaction in the `DiscardFunding` event is
replaced with a dummy empty transaction.
This commit checks if `is_manual_broadcast` is true and
stores the funding OutPoint in the DiscardFunding event instead.
Matt Corallo [Thu, 22 Aug 2024 19:15:34 +0000 (19:15 +0000)]
Split `ConfirmationTarget::OnChainSweep` into urgent and non-urgent
When we force-close a channel, occasionally its due to feerate
disagreements or other non-HTLC-related issues. In those cases,
there's no reason to use a very urgent feerate estimate - we don't
have any timers expiring soon.
Instead, we should give users the information they need to be more
economical on fees in this case, which we do here by splitting
`OnChainSweep` into `UrgentOnChainSweep` and
`NonUrgentOnChainSweep` `ConfirmationTarget`s.
Matt Corallo [Thu, 22 Aug 2024 15:25:56 +0000 (15:25 +0000)]
Add a new `ConfirmationTarget::MaximumFeeEstimate`
When we broke `ConfirmationTarget` out into task-specific names, we
left `MaxDustHTLCExposure::FeeRateMultiplier` as using the "when we
broadcast feerate" as we were mostly concerned about the dust
thresholds on outbound channels where we pick the fee and drive our
own funds to dust.
In 51bf78d604b28fe189171e1b976fe87cbb2614b7, that changed to
include transaction fees on both inbound and outbound channels in
our dust exposure amount, but we continued to use
`ConfirmationTarget::OnChainSweep` for the fee estimator threshold.
While the `MaxDustHTLCExposure::FeeRateMultiplier` value is quite
conservative and shouldn't lead to force-closures unless feerate
estimates disagree by something like 500 sat/vB (with only one HTLC
active in a channel), this happened on Aug 22 when feerates spiked
from 4 sat/vB to over 1000 sat/vB in one block.
To avoid simple feerate estimate horizons causing this in the
future, here we add a new
`ConfirmationTarget::MaximumFeeEstimate` which is used for dust
calculations. This allows users to split out the estimates they use
for checking counterparty feerates from the estimates used for
actual broadcasting.
Matt Corallo [Mon, 26 Aug 2024 19:25:46 +0000 (19:25 +0000)]
Return owned `String`s for onion message message types
Returning a reference from a trait method is relatively difficult
to map in bindings and is currently handled by storing the object
in the trait instance, returning a reference to the local field.
This is fine when the object we're returning only needs to live as
long as the trait, but when it needs to be `'static` (as is the
case for onion message `msg_type`s), there's not really a good way
to map them at all.
Instead, here, condition on `#[cfg(c_bindings)]` we return a fully
owned `String`. This is obviously relatively less effecient, but
the extra allocation and `memcpy` isn't the end of the world,
especially given it should be released relatively quickly.
Note that this breaks doctests in with `c_bindings`.
Matt Corallo [Thu, 22 Aug 2024 01:31:07 +0000 (01:31 +0000)]
Add a `MessageSendInstructions::ForReply`
In order to allow onion message handlers to reply asynchronously
without introducing a circular dependency graph, the message
handlers need to be able to send replies described by
`MessageSendInstructions`. This allows them to send replies via the
normal message queuing (i.e. without making a function call to
`OnionMessenger`).
Here we enable that by adding a `MessageSendInstructions::ForReply`
variant which holds `ReplyInstruction`s.
Matt Corallo [Wed, 21 Aug 2024 19:10:46 +0000 (19:10 +0000)]
3/3 Use `MessageSendInstructions` instead of `PendingOnionMessage`
Now that the `MessageRouter` can `create_blinded_paths` forcing
callers of the `OnionMessenger` to provide it with a reply path up
front is unnecessary complexity, doubly so in message handlers.
Here we take the next step towards untangling that, moving from
`PendingOnionMessage` to `MessageSendInstructions` for the outbound
message queue in `AsyncPaymentsMessageHandler`. Better, we can also
drop the `c_bindings`-specific message queue variant, unifying the
APIs.
Matt Corallo [Thu, 22 Aug 2024 01:42:55 +0000 (01:42 +0000)]
2/3 Use `MessageSendInstructions` instead of `PendingOnionMessage`
Now that the `MessageRouter` can `create_blinded_paths` forcing
callers of the `OnionMessenger` to provide it with a reply path up
front is unnecessary complexity, doubly so in message handlers.
Here we take the next step towards untangling that, moving from
`PendingOnionMessage` to `MessageSendInstructions` for the outbound
message queue in `OffersMessageHandler`. Better, we can also drop
the `c_bindings`-specific message queue variant, unifying the APIs.
Because `ChannelManager` needs to actually control the reply path
set in individual messages, however, we have to halfway this patch,
adding a new `MessageSendInstructions` variant that allows
specifying the `reply_path` explicitly. Still, because other message
handlers are moving this way, its nice to be consistent.
Matt Corallo [Thu, 22 Aug 2024 01:41:27 +0000 (01:41 +0000)]
1/3 Use `MessageSendInstructions` instead of `PendingOnionMessage`
Now that the `MessageRouter` can `create_blinded_paths` forcing
callers of the `OnionMessenger` to provide it with a reply path up
front is unnecessary complexity, doubly so in message handlers.
Here we take the first step towards untangling that, moving from
`PendingOnionMessage` to `MessageSendInstructions` for the outbound
message queue in `CustomMessageHandler`. Better, we can also drop
the `c_bindings`-specific message queue variant, unifying the APIs.
Matt Corallo [Thu, 22 Aug 2024 18:43:33 +0000 (18:43 +0000)]
Give `MessageSendInstructions` any `Destination`, not only Blinded
In the next commit we'll use `MessageSendInstructions` for all
messages, so it needs the ability to take any `Destination`, not
only a `Destination::Blinded`.
Matt Corallo [Thu, 22 Aug 2024 01:35:18 +0000 (01:35 +0000)]
Pull the guts of `ResponseInstruction` into a new enum
In the coming commits we'll use the `ResponseInstruction` enum's
contents for all messages, allowing message senders to rely on the
in-`OnionMessegner` reply path selection logic.
In order to do so and avoid users confusing the new
`MessageSendInstructions` for `ResponseInstruction`, we leave
`ResponseInstruction` as a now-unconstructible struct which wraps a
`MessageSendInstructions`.
Matt Corallo [Wed, 21 Aug 2024 15:32:14 +0000 (15:32 +0000)]
Remove message type bound on `ResponseInstruction`
Our onion message handlers generally have one or more methods which
return a `ResponseInstruction` parameterized with the expected
message type (enum) of the message handler.
Sadly, that restriction is impossible to represent in our bindings -
our bindings concretize all LDK structs, enums, and traits into a
single concrete instance with generics set to our concrete trait
instances (which hold a jump table). This prevents us from having
multiple instances of `ResponseInstruction` structs for different
message types.
Our bindings do, however, support different parameterizations of
standard enums, including `Option`s and tuples.
In order to support bindings for the onion message handlers, we
are thus forced into std types bound by expected message types,
which we do here by making `ResponseInstruction` contain only the
instructions and generally using it as a two-tuple of
`(message, ResponseInstruction)`.
Matt Corallo [Wed, 21 Aug 2024 16:46:24 +0000 (16:46 +0000)]
Disambiguate blinded path `ForwardNode`s between payment + message
We currently have two structs with identical names in our public
API - `blinded_path::message::ForwardNode` and
`blinded_path::payment::ForwardNode`. This makes the API somewhat
awkward to use - users have to try (and fail) to import
`ForwardNode` twice only to then have to change their imports.
More importantly, however, this makes the API very hard to use in
some bindings languages where rename-imports or module imports
aren't available.
Matt Corallo [Sat, 17 Aug 2024 21:23:16 +0000 (21:23 +0000)]
Remove uneccessary `std` bounds on many tests
We never actually build with `#![no_std]` in tests as Rust does
not support it. Thus, many tests have spurious `std` feature gates
when they run just fine without them. Here we remove those gates,
though note that tests that depend on behavior elsewhere in the
codebase which is `std`-gated obviously need to retain their
feature gates.
Matt Corallo [Sat, 17 Aug 2024 15:59:34 +0000 (15:59 +0000)]
Drop `std::Error` impl for `PeerHandleError`
Not sure why we ever really had this, no one really ever bounds
anything on `std::Error` and its kinda a dead type, so there's no
need for us to `impl` it for our types.
Matt Corallo [Fri, 16 Aug 2024 22:36:30 +0000 (22:36 +0000)]
Simplify `Instant` mocking in outbound payments
To handle `std` and `no-std` concepts of time in scoring, we'd
originally written a generic `Time` trait which we could use to
fetch the current time, observe real (not wall-clock) elapsed time,
and serialize the time.
Eventually, scoring stopped using this `Time` trait but outbound
payment retry expiry started using it instead to mock time in
tests.
Since scoring no longer uses the full features which required the
`Time` trait, we can substantially simplify by just having the
mocking option.
Matt Corallo [Fri, 16 Aug 2024 20:46:37 +0000 (20:46 +0000)]
Remove note about `std`/`no-std` scorer serialization compat
In 81389dee306d960a8030bec5ffa304004148ce85 we removed a note about
mixing the `std` and `no-std` feature when de/serializing
`ProbabilisticScorer`s but forgot to note that there was a second
copy of that note in the module documentation.
Elias Rohrer [Tue, 20 Aug 2024 11:47:51 +0000 (13:47 +0200)]
Rename `ChannelDetails::is_public` to `is_announced`
Referring to announced/unannounced channels as 'public'/'private'
regularly leads to confusion on what they are and when which should be
used. To avoid perpetuating this confusion, we should avoid referring to
announced channels as 'public' in our API.
Here we rename `ChannelDetails::is_public` (which breaks
previously-released API) to align it with the changes in
`OpenChannelRequest`.
Elias Rohrer [Tue, 20 Aug 2024 11:43:40 +0000 (13:43 +0200)]
Rename `OpenChannelRequest::is_public` to `is_announced`
Referring to announced/unannounced channels as 'public'/'private'
regularly leads to confusion on what they are and when which should be
used. To avoid perpetuating this confusion, we should avoid referring to
announced channels as 'public' in our API.
Here we rename the recently introduced field in `OpenChannelRequest`
(which doesn't break released API), and will align the pre-existing
instances of `is_public` in the following commit (which will break API).
Jeffrey Czyz [Fri, 16 Aug 2024 02:37:03 +0000 (21:37 -0500)]
Use a single iterator to construct a BlindedPath
Instead of using separate iterators for pubkeys and TLVs, use an
iterator that yields a tuple of them. This allows for holding a mutable
reference to the TLVs such that they can be padded. With two iterators,
the pubkey iterator would have a reference to the ForwardNode slice when
constructing a payment path. However, this would prevent holding mutable
references in the TLVs iterator.
Jeffrey Czyz [Fri, 16 Aug 2024 01:05:33 +0000 (20:05 -0500)]
Generalize build_keys_helper
When constructing a blinded path, two iterators are used: one for the
pubkeys and another for Writeable TLVs. The first iterator is used in
the build_keys_helper utility function while the second is used inside
of a callback. Update this utility to work on any type that can be
borrowed as a PublicKey. This allows for using a single iterator of
tuples, which is necessary for padding the hops without additional
allocations and clones.
Jeffrey Czyz [Thu, 15 Aug 2024 23:06:46 +0000 (18:06 -0500)]
Add a construct_keys_callback for blinded paths
When constructing a BlindedPath, an Option<Destination> parameter to
construct_keys_callback is never passed. Make a separate utility
function that doesn't take this parameter. This allows for using the
build_keys_helper macro with a Iterator yielding items other than
PublicKey.
Jeffrey Czyz [Thu, 15 Aug 2024 22:56:17 +0000 (17:56 -0500)]
Use PublicKey values in construct_keys_callback
Instead of accepting iterators yielding PublicKey by reference in
utils::construct_keys_callback, take iterators yielding values since
these implement Copy and need to be copied anyway. This will help avoid
a situation when padding where both a reference and mutable reference
are needed.
Jeffrey Czyz [Thu, 15 Aug 2024 22:36:55 +0000 (17:36 -0500)]
Refactor helper macro from construct_keys_callback
When constructing a BlindedPath, utils::construct_blinded_hops uses two
iterators. However, this makes it difficult to pad blinded hops to equal
sizes without allocating a vector or cloning data. Refactor the
construct_keys_callback utility function so that is can be used with an
Iterator with different Item types. This allows using a single Iterator
of tuples while still supporting its use only with pubkeys.
G8XSU [Tue, 20 Aug 2024 17:23:18 +0000 (10:23 -0700)]
Doc Clarity: Handling gaps in persisted ChannelMonitorUpdates
If there are any gaps in the persisted [`ChannelMonitorUpdate`]s,
implementer can safely ignore [`ChannelMonitorUpdate`]s after the gap and load without them.
Since, acc. to channel-manager, we have only made progress if all contiguos
ChannelMonitorUpdates have been persisted.
Duncan Dean [Fri, 16 Aug 2024 09:34:55 +0000 (11:34 +0200)]
Deprecate AvailableBalances::balance_msat
The ChannelMonitor::get_claimable_balances method provides a more
straightforward approach to the balance of a channel, which satisfies
most use cases. The computation of AvailableBalances::balance_msat is
complex and originally had a different purpose that is not applicable
anymore. We deprecate AvailableBalances::balance_msat now and will remove
it in a following release.
Co-authored-by: Willem Van Lint <noreply@wvanlint.dev>
This test was added some time ago in 0c034e9a82e4339fb32af9da63832ac2a64abb0b, but never made any sense.
`PeerManager::process_events` will go around its loop as many times
is required to ensure we've always processed all events which were
pending prior to a `process_events` call, so having a test that
checks that we never go around more than twice is obviously broken.
And, indeed, in CI this tests fails with some regularity.
Instead, the test here is changed to ensure that we detectably go
around the loop again at least once.