- Wait until HTLCs are irrevocably removed before cleaning up their
data structures (MPP and forwarding)
- keep methods maybe_cleanup_mpp and maybe_cleanup_forwarding separate
- perform cleanup in htlc_switch, so that process_unfulfilled_htlc
has less side effects
- In htlc_switch, we blank the onion_packet_hex field to signal that
an HTLC has been processed. An item of chan.unfulfilled_htlcs may
go through 4 stages:
- 1. not forwarded yet: (None, onion_packet_hex)
- 2. forwarded: (forwarding_key, onion_packet_hex)
- 3. processed: (forwarding_key, None), not irrevocably removed yet
- 4. done: (forwarding_key, None), irrevocably removed
- in test_lnpeer, an extra iteration of htlc_switch has been added to
trampoline forwarding tests
If we accept a MPP and we forward the payment (trampoline or swap),
we need to persist the payment accepted status, or we might wrongly
release htlcs on the next restart.
lnworker.received_mpp_htlcs used to be cleaned up in maybe_cleanup_forwarding,
which only applies to forwarded payments. However, since we now
persist this dict, we need to clean it up also in the case of
payments received by us. This part of maybe_cleanup_forwarding has
been migrated to lnworker.maybe_cleanup_mpp
This will be useful if we decide to ship lntransport as a separate
package. It is also a conceptual cleanup.
Notes:
- lntransport still requires crypto.py
- parsing node id from a bolt11 invoice is not supported.
Values for exponential search are based on available fee budget:
we try with budget/64, budget/32, ..., budget/1 (spread uniformly among the selected Trampoline Forwarders).
Hence, if we make the fee budget configurable, that will usefully affect the trampoline fees as well.
related https://github.com/spesmilo/electrum/issues/9033
Instead of some functions operating with hex strings,
and others using bytes, this consolidates most things to use bytes.
This mainly focuses on bitcoin.py and transaction.py,
and then adapts the API usages in other files.
Notably,
- scripts,
- pubkeys,
- signatures
should be bytes in almost all places now.
This gives more time for the client to come back online.
see https://github.com/spesmilo/electrum/issues/8940
- re note on submarine_swaps.py#L53:
lnpeer.Peer.maybe_fulfill_htlc only checks against MIN_FINAL_CLTV_DELTA_ACCEPTED(=144),
so this increased cltv_delta is not enforced when receiving the htlc on ln.
It is put in the invoice, so the sender is supposed to honour it ofc.
It would be nice to enforce it (make the check in maybe_fulfill_htlc dependent on
what was in the invoice).
- scenario:
- reuse same seed between two devices, LN enabled on both
- open channel using device1, import chan backup on device2
- local-force-close channel using device1
- tx1 (ctx) gets into mempool (or even mined), tx2 (sweep tx for to_local) is a local (future) tx
- history tab on device1 shows tx1 and tx2
- history tab on device2 was showing only tx2, and no info about tx1
- device2 knows about tx1, it was just not showing it previously.
With this commit, tx1 is now shown in the history.
- note: tx1 might linger in the mempool for an indeterminate amount of time, or even become local.
During that time, it is confusing on device2 not to show any feedback. Also,
if tx1 becomes local, it is useful to be able to rebroadcast it.
The existing logic of only updating the fee if it is not within 2x of
the current 2-block-eta does not work well for the current mempool.
The current mempool looks a bit weird: you need ~20 sat/vbyte to even get into it,
but there is only around 5 MB of txs paying >25 sat/vbyte.
The estimates look like this:
```
>>> config.fee_estimates
{144: 25764, 25: 27075, 10: 34538, 5: 34538, 2: 34538}
```
This commit changes the logic so that we send update_fee if the old rate is
- below 75% of the current 2-block-eta (instead of 50%), or
- below the 25-block-eta
lnworker.suggest_splits for non-trampoline case tries to split amts over 5000 sat
but mpp_split.suggest_splits does not return splits where any part is smaller than 10000 sat.
So in practice, without trampoline, invoices between 5k and ~20k sats could not be paid.
This suggests that the logic should not be scattered in multiple places but merged into mpp_split.py...
This commit just does a quick fix though, to try again without splitting if there was no solution.
- all forwarding types use the same flow
- forwarding callback returns a htlc_key or None
- forwarding info is persisted in lnworker:
- ongoing_forwardings
- downstream to upstream htlc_key
- htlc_key -> error_bytes
- a node scid alias is derived from the node ID
- the channel opening fee is sent in a TLV field of open_channel
- the server requires htlc settlement before broadcasting
(server does not trust client)
- introduce PaymentFeeBudget, which contains limits for fee budget and cltv budget
- when splitting a payment,
- the fee budget is linearly distributed between the parts
- this resolves a FIXME in lnrouter ("FIXME in case of MPP")
- the cltv budget is simply copied
- we could also add other kinds of budgets later, e.g. for the num in-flight htlcs
- resolves TODO in lnworker ("todo: compare to the fee of the actual route we found")
The convention is that edges (start_node -> edge_node) store
the policy/fees for the *start_node*.
This is what the non-trampoline edges were already using (for a long time),
but the trampoline ones were off-by-one (policy was for end_node),
which was then worked around in multiple places, to correct for...
i.e. I think because of all the workarounds, there was no actual bug,
but it was just very confusing.
Also note that the prior usage of trampoline edges would not work if
we (sender) were not directly connected to a TF (trampoline-forwarder)
but had extra edges in the route to even get to the first TF.
Having the policy corresponding to the start_node of the edge would work
even in that case.
ThomasV
Sander van Grieken
SomberNight
jinjiadu
MrNaif2018