`lightning-transaction-sync`'s `esplora-async` dependency
indirectly depends on `tokio-util`, which, like tokio, recently
bumped its MSRV.
Here we update `ci/ci-tests.sh` to pin `tokio-util` to make MSRV
builds pass.
`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.
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.
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.
There's not a lot of reason to do this, but sadly the bindings
don't currently resolve wildcard imports at all, and I don't want
to deal with implementing it right now.
In 11ab302087 we accidentally removed
the `not(ldk_bench)` bound before outputting logs to stderr, which
we restore here. Instead of simply ignoring logs in benchmarks,
which we did previously, we instead format logs (in a way that LLVM
will not optimize out).
There's no reason why we should be filtering our logs during tests.
It seems mostly like the `TestLogger` was written more as a sample
logger than a test logger, but these days we have samples, so we
should just drop the filtering and show everything.
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`.
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.
`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.
In 5e874c3dc9 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.
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.
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.
5e874c3dc9 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.
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.
[1] https://github.com/lightningdevkit/rust-lightning/pull/3024
Link: https://github.com/lightningdevkit/rust-lightning/issues/3164
Suggested-by: TheBlueMatt
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
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.
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 51bf78d604, 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.
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`.
