Rename parameters used when calculating success probability to make it
clear that the total mount in-flight should be used rather than the
payment amount.
Commit df52da7b31 modified
ProbabilisticScorer to apply some penalty amount multipliers to the
total amount flowing over the channel. However, the commit updated the
docs for base_penalty_amount_multiplier_msat even though that behavior
didn't change. This commit reverts those docs.
Commit df52da7b31 modified
ProbabilisticScorer to apply some penalty amount multipliers (e.g.,
liquidity_penalty_amount_multiplier_msat) to the total amount flowing
over the channel (i.e., including inflight HTLCs), not just the payment
in question. This led to over-penalizing in-use channels. Instead, only
apply the total amount when calculating success probability.
In a previous commit we updated the fee-bump-rate of claims against
HTLC timeouts on counterparty commitment transactions so that
instead of immediately attempting to bump every block we consider
the fact that we actually have at least `MIN_CLTV_EXPIRY_DELTA`
blocks to do so, and bumping at the appropriate rate given that.
Here we test that by adding an extra check to an existing test
that we do not bump in the very next block after the HTLC timeout
claim was initially broadcasted.
For outbound HTLCs, the counterparty can spend the output
immediately. This fixes the `counterparty_spendable_height` in the
`PackageTemplate` claiming outbound HTLCs on local commitment
transactions, which was previously spuriously set to the HTLC
timeout (at which point *we* can claim the HTLC).
Now that the module only contains some implementations of
serialization for the `lightning_types::features` structs, there's
no reason for it to be public.
This renames the field in `PackageTemplate` which describes the
height at which a counterparty can make a claim to an output to
match its actual use.
Previously it had been set based on when a counterparty can claim
an output but also used for other purposes. In the previous commit
we cleaned up its use for fee-bumping-rate, so here we can rename
it as it is now only used as the `counteraprty_spendable_height`.
`PackageTemplate::get_height_timer` is used to decide when to next
bump our feerate on claims which need to make it on chain within
some window. It does so by comparing the current height with some
deadline and increasing the bump rate as the deadline approaches.
However, the deadline used is the `counterparty_spendable_height`,
which is the height at which the counterparty might be able to
spend the same output, irrespective of why. This doesn't make sense
for all output types, for example outbound HTLCs are spendable by
our counteraprty immediately (by revealing the preimage), but we
don't need to get our HTLC timeout claims confirmed immedaitely,
as we actually have `MIN_CLTV_EXPIRY` blocks before the inbound
edge of a forwarded HTLC becomes claimable by our (other)
counterparty.
Thus, here, we adapt `get_height_timer` to look at the type of
output being claimed, and adjust the rate at which we bump the fee
according to the real deadline.
Now that we don't store the confirmation height of the inputs
being spent, passing the current height to
`PackageTemplate::build_package` is useless - we only use it to set
the height at which we should next bump the fee, but we just want
it to be "next block", so we might as well use `0` and avoid the
extra argument. Further, in one case we were already passing `0`,
so passing the argument is just confusing as we can't rely on it
being set.
Note that this does remove an assertion that we never merge
packages that were crated at different heights, and in the future
we may wish to do that (as there's no specific reason not to), but
we do not currently change the behavior.
This has never been used, and its set to a fixed value of zero for
HTLCs on local commitment transactions making it impossible to rely
on so might as well remove it.
This function was very confusing - its used to determine by when
we have to stop aggregating this claim with others as it starts to
be at risk of pinning due to the counterparty's ability to spend
the output.
It is not ever used as a timelock for a transaction, and thus its
name is very confusing.
Instead we rename it `counterparty_spendable_height`.
We don't actually care if a confirmed transaction claimed other
outputs, only that it claimed a superset of the outputs in the
pending claim we're looking at. Thus, the variable to detect that
is renamed `is_claim_subset_of_tx` instead of `are_sets_equal`.
Users commonly want to know what their balance was when a channel
was closed, which this provides in a somewhat simplified manner.
It does not consider pending HTLCs and will always overstate our
balance by transaction fees.
In aa2f6b47df we refactored
`lightning-invoice` de/serialization to use the new version of
`bech32`, but ended up adding one unnecessary allocation in our
offers logic, which we drop here.
A `DNSResolverMessageHandler` which handles resolution requests
should want the `NodeFeatures` included in the node's
`node_announcement` to include `dns_resolver` to indicate to the
world that it provides that service. Here we enable this by
requesting extra feature flags from the `DNSResolverMessageHandler`
in the features `OnionMessenger`, in turn, provides to
`PeerManager` (which builds the `node_announcement`).
While `message_received` purports to be called on every message,
prior to the message, doing so on `Init` messages means we have to
call `message_received` while holding the per-peer mutex, which
can cause some lock contention.
Instead, here, we call `message_received` after processing `Init`
messages (which is probably more useful anyway - the peer isn't
really "connected" until we've processed the `Init` messages),
allowing us to call it unlocked.
`creates_and_pays_for_offer_with_retry` intends to check that we
re-send a BOLT 12 `invoice_request` in response to a
`message_received` call, but doesn't actually test that there were
no messages in the outbound buffer after the initial send, which we
do here.
This adds a new utility struct, `OMNameResolver`, which implements
the core functionality required to resolve Human Readable Names,
namely generating `DNSSECQuery` onion messages, tracking the state
of requests, and ultimately receiving and verifying `DNSSECProof`
onion messages.
It tracks pending requests with a `PaymentId`, allowing for easy
integration into `ChannelManager` in a coming commit - mapping
received proofs to `PaymentId`s which we can then complete by
handing them `Offer`s to pay.
It does not, directly, implement `DNSResolverMessageHandler`, but
an implementation of `DNSResolverMessageHandler` becomes trivial
with `OMNameResolver` handling the inbound messages and creating
the messages to send.
BIP 353 `HumanReadableName`s are represented as `₿user@domain` and
can be resolved using DNS into a `bitcoin:` URI. In the next
commit, we will add such a resolver using onion messages to fetch
records from the DNS, which will rely on this new type to get name
information from outside LDK.
This creates the initial DNSSEC proof and query messages in a new
module in `onion_message`, as well as a new message handler to
handle them.
In the coming commits, a default implementation will be added which
verifies DNSSEC proofs which can be used to resolve BIP 353 URIs
without relying on anything outside of the lightning network.
When we make a DNSSEC query with a reply path, we don't want to
allow the DNS resolver to attempt to respond to various nodes to
try to detect (through timining or other analysis) whether we were
the one who made the query. Thus, we need to include a nonce in the
context in our reply path, which we set up here by creating a new
context type for DNS resolutions.
We often process many gossip messages in parallel across different
peer connections, making the `NetworkGraph` mutexes fairly
contention-sensitive (not to mention the potential that we want to
send a payment and need to find a path to do so).
Because we need to look up a node's public key to validate a
signature on `channel_update` messages, we always need to take a
`NetworkGraph::channels` lock before we can validate the message.
For simplicity, and to avoid taking a lock twice, we'd always
validated the `channel_update` signature while holding the same
lock, but here we address the contention issues by doing a
`channel_update` validation in three stages.
First we take a read lock on `NetworkGraph::channels` and check if
the `channel_update` is new, then release the lock and validate the
message signature, and finally take a write lock, (re-check if the
`channel_update` is new) and update the graph.
