The previous commit wraps the background thread's JoinHandle in an
Option. Providing a dedicated method to join hides this implementation
detail from users.
Previously, we could fail to generate a new commitment transaction
but it simply indicated we had gone to doule-claim an HTLC. Now
that double-claims are returned instead as Ok(None), we should
handle the error case and fail the channel, as the only way to hit
the error case is if key derivation failed or the user refused to
sign the new commitment transaction.
This also resolves an issue where we wouldn't inform our
ChannelMonitor of the new payment preimage in case we failed to
fetch a signature for the new commitment transaction.
When receiving an update_fulfill_htlc message, we immediately
forward the claim backwards along the payment path before waiting
for a full commitment_signed dance. This is great, but can cause
duplicative claims if a node sends an update_fulfill_htlc message,
disconnects, reconnects, and then has to re-send its
update_fulfill_htlc message again.
While there was code to handle this, it treated it as a channel
error on the inbound channel, which is incorrect - this is an
expected, albeit incredibly rare, condition. Instead, we handle
these double-claims correctly, simply ignoring them.
With debug_assertions enabled, we also check that the previous
close of the same HTLC was a fulfill, and that we are not moving
from a HTLC failure to an HTLC claim after its too late.
A test is also added, which hits all three failure cases in
`Channel::get_update_fulfill_htlc`.
Found by the chanmon_consistency fuzzer.
Without stopping the thread when BackgroundProcessor is dropped, it will
run free. In the context of language bindings, it is difficult to know
how long references held by the thread should live. Implement Drop to
stop the thread just as is done when explicitly calling stop().
The specific error from the ChannelManager persister is not asserted for
in test_persist_error. Rather, any error will do. Update the test to use
BackgroundProcessor::stop and assert for the expected value.
When there are fewer known `from` feature bytes than known `to` feature
bytes, an index-out-of-bounds error can occur if the `from` features
have unknown features set in a byte past the greatest known `from`
feature byte.
This was reported by a user when trying to send a payment using the LDK
sample (specifically during route generation when translating a Features
from one context to another)
The problem was we didn't check T::KNOWN_FEATURE_MASK vec length before
indexing into it, due likely to the assumption that known feature vec
lengths are the same across contexts, when they may not be
As the variable name implies holder_selected_chan_reserve_msat is
intended to be in millisatoshis, but is instead calculated in
satoshis.
We fix that error here and update the relevant tests to more
accurately calculate the expected reserve value and test both
success and failure cases.
Bug discovered by chanmon_consistency fuzz target.
Instead of interpreting the backwards compatibility data in Channel
serialization, use the serialization version bump present in 0.0.99
as the flag to indicate if a channel should be read in backwards
compatibility.
Private nodes should never wish to forward HTLCs at all, which we
support here by disabling forwards out over private channels by
default. As private nodes should not have any public channels, this
suffices, without allowing users to disable forwarding over
channels announced in the routing graph already.
Closes#969
Currently the base fee we apply is always the expected cost to
claim an HTLC on-chain in case of closure. This results in
significantly higher than market rate fees [1], and doesn't really
match the actual forwarding trust model anyway - as long as
channel counterparties are honest, our HTLCs shouldn't end up
on-chain no matter what the HTLC sender/recipient do.
While some users may wish to use a feerate that implies they will
not lose funds even if they go to chain (assuming no flood-and-loot
style attacks), they should do so by calculating fees themselves;
since they're already charging well above market-rate,
over-estimating some won't have a large impact.
Worse, we current re-calculate fees at forward-time, not based on
the fee we set in the channel_update. This means that the fees
others expect to pay us (and which they calculate their route based
on), is not what we actually want to charge, and that any attempt
to forward through us is inherently race-y.
This commit adds a configuration knob to set the base fee
explicitly, defaulting to 1 sat, which appears to be market-rate
today.
[1] Note that due to an msat-vs-sat bug we currently actually
charge 1000x *less* than the calculated cost.
