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.
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.
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.
DefaultRouter::create_blinded_payment_paths may creat a one-hop blinded
path with the recipient as the introduction node. Update the privacy
section of DefaultRouter's docs to indicate this as is done in the docs
for DefaultMessageRouter.
ChannelManager is parameterized by a Router, which must also implement
MessageRouter. Instead, add a MessageRouter parameter such that the
Router and MessageRouter traits can be de-coupled. This simplifies using
something other than DefaultMessageRouter, which DefaultRouter currently
delegates to.
We expect our users to have fully idempotent `Event` handling as we
may replay events on restart for one of a number of reasons. This
isn't a big deal as long as all our events have some kind of
identifier users can use to check if the `Event` has already been
handled.
For outbound payments, this is the `PaymentId` they provide in the
send methods, however for inbound payments we don't have a great
option.
`PaymentHash` largely suffices - users can simply always claim in
response to a `PaymentClaimable` of sufficient value and treat a
`PaymentClaimed` event as duplicate any time they see a second one
for the same `PaymentHash`. This mostly works, but may result in
accepting duplicative payments if someone (incorrectly) pays twice
for the same `PaymentHash`.
Users could also fail for duplicative `PaymentClaimable` events of
the same `PaymentHash`, but doing so may result in spuriously
failing a payment if the `PaymentClaimable` event is a replay and
they never saw a corresponding `PaymentClaimed` event.
While none of this will result in spuriously thinking they've been
paid when they have not, it does result in some pretty awkward
semantics which we'd rather avoid our users having to deal with.
Instead, here, we add a new `PaymentId` which is simply an HMAC of
the HTLCs (as Channel ID, HTLC ID pairs) which were included in the
payment.
In the next commit we'll start generating `PaymentId`s for inbound
payments randomly by HMAC'ing the HTLC set of the payment. Here we
start by defining the HMAC secret for these HMACs.
This requires one small test adaptation and a full_stack_target
fuzz change because it changes the RNG consumption.
In the next commit we'll change the order of HTLCs in
`PaymentClaim[able,ed]` events. This shouldn't break anything, but
our current functional tests check that the HTLCs are provided in
the order they expect (the order they were received). Instead, here
we only validate that each claimed HTLC matches one expected path.
429cbe1a06 merged a PR that renamed
Offer::signing_pubkey to Offer::issuer_signing_pubkey. However, there was a
silent rebase conflict and a test added as part of
1059f5ffc5 did not get the memo and used the old
method name, breaking the test build.
This fixes the following bug:
- An outbound payment is AwaitingInvoice
- We receive an invoice and lock the HTLCs into the relevant ChannelMonitors
- The monitors are successfully persisted, but the ChannelManager fails to
persist, so the outbound payment remains AwaitingInvoice
- We restart, causing the channels to close due to a stale ChannelManager
- We receive a duplicate invoice, and attempt to pay it again due to the
payment still being AwaitingInvoice in the stale ChannelManager
After the fix for this, we will notice that the payment is already locked into
the monitor on startup and transition the incorrectly-AwaitingInvoice payment
to Retryable, which prevents double-paying on duplicate invoice receipt.
The spec was recently changed to use offer_issuer_id instead of
offer_node_id. LDK always used signing_pubkey to avoid confusion with a
node_id. Rename it to issuer_signing_pubkey now as InvoiceRequest and
Bolt12Invoice will have similarly named methods in upcoming commits.
