Skip to content

Commit cc45ddc

Browse files
f321xf321x
committed
wip: todo
1 parent 1ccce69 commit cc45ddc

2 files changed

Lines changed: 225 additions & 0 deletions

File tree

‎review-f5053da71-onion-message-direct-connect.md‎

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,83 @@
1+
# Review: `f5053da71` — onion_message: perform a direct peer connection for sending request
2+
3+
- **Commit:** `f5053da7104d9080a9419ca81fd6aef83bc2b77d`
4+
- **Author:** Sander van Grieken — Mon Nov 24 15:16:20 2025 +0100
5+
- **Branch:** `bolt12_2` (not merged to `master`; `master` is the pre-commit lineage)
6+
- **Files:** `electrum/onion_message.py`, `electrum/simple_config.py`, `tests/test_onion_message.py`
7+
- **Reviewed:** the actual post-commit tree, not the working copy.
8+
9+
## Verdict
10+
11+
**Reasonable and a net improvement — but only for the single-destination case.** The headline goal in the commit message ("when we have tried *all* blinded paths…") is **not** actually achieved by the control flow: for ≥2 destinations the new bookkeeping is unreachable dead code, and behavior is unchanged from before.
12+
13+
## What it does
14+
15+
1. Adds a config toggle `ONION_MESSAGE_OPEN_DIRECT_CONNECTIONS` (default `True`) with a clear privacy rationale.
16+
2. Makes `_send_pending_message` async and wraps `send_onion_message_to` in `try/except NoRouteFound`. On `NoRouteFound` it marks the destination as failed (`route_not_found_for[dest_index] = True`) and — *if all destinations have failed* and the toggle is on and a `peer_address` hint exists — opens a direct connection via `add_peer` instead of re-raising. Otherwise it re-raises.
17+
3. Removes the old direct-connect block from `process_send_queue`.
18+
19+
## What was there "before"
20+
21+
The pre-commit code already did a direct connection, in `process_send_queue`:
22+
23+
```python
24+
except BaseException as e:
25+
req.future.set_exception(copy.copy(e)) # <-- request already FAILED here
26+
if isinstance(e, NoRouteFound) and e.peer_address:
27+
await self.lnwallet.lnpeermgr.add_peer(str(e.peer_address)) # <-- too late for THIS request
28+
```
29+
30+
The old version called `add_peer` **after** `set_exception` had already failed the request, and there was no resubmit afterward (resubmit only happens in the `else`/success branch). So the connection it opened was orphaned — it could only help a *later, separate* request to the same node (if some higher-level caller retried).
31+
32+
**The new code is a genuine improvement here**: it connects *before* failing and then lets `process_send_queue` resubmit, so the same request is retried over the now-direct connection and can actually succeed. This is the real win, and it's correct for a single destination.
33+
34+
There's also a quiet robustness fix: in the old code, if `add_peer` *raised* inside the `except` handler of `process_send_queue`, it would propagate out of the `while` loop and tear down the entire send-queue task. In the new code `add_peer` runs inside `_send_pending_message`, so a raise is caught by `process_send_queue` and only fails the one request. Good.
35+
36+
## Main issue — the multi-path fallback is unreachable (`electrum/onion_message.py:733-740`)
37+
38+
```python
39+
except NoRouteFound as e:
40+
req.route_not_found_for[dest_index] = True
41+
if all(req.route_not_found_for) and self.send_direct_connect_fallback and e.peer_address:
42+
... add_peer ...
43+
else:
44+
raise
45+
```
46+
47+
Trace it for **N ≥ 2** destinations (`route_not_found_for = [None, None]`):
48+
49+
- Attempt 1 → dest index 0 → `NoRouteFound` → `route_not_found_for = [True, None]` → `all([True, None])` is **False** → `raise`.
50+
- `process_send_queue` catches it → `set_exception` → request is **permanently failed**; `_wait_task` removes it. There is no resubmit (resubmit is only in the no-exception `else` branch).
51+
- Attempt 2 **never happens**, so index 1 is never tried and `all(...)` is never True.
52+
53+
`all(req.route_not_found_for)` can therefore only ever be satisfied when there is exactly **one** destination. For multiple blinded paths the request dies on the first path's `NoRouteFound` — identical to the old behavior — and the `route_not_found_for` list is pure overhead. The `TODO: this will only attempt direct connection to the last blinded path ip node` comment is moot: it never reaches *any* path's direct connection for N≥2.
54+
55+
**Root cause:** the code conflates "this path failed, try the next one" with "fail the whole request." To actually deliver the stated goal, an intermediate `NoRouteFound` (when `not all(...)`) should let the round-robin advance — i.e. return without raising so `process_send_queue` resubmits and `get_next_destination` moves to the next path — and only the all-paths-exhausted case should trigger direct-connect or final failure. (That in turn raises the question of how the final "all failed, no fallback" case should fail — fast `NoRouteFound` vs. eventual `Timeout` — which needs a deliberate decision.)
56+
57+
This is purely forward-looking right now: there are **no production callers** of `submit_send` on the branch yet (only tests), and `get_next_destination` round-robin + the class docstring make multi-path a clearly intended BOLT12 use case. So it's worth fixing before consumers are wired up, but it isn't breaking anything today.
58+
59+
## Secondary observations
60+
61+
- **`NoRouteFound` → `Timeout` substitution, and no reset of `route_not_found_for`.** For a single dest with fallback on, once index 0 is marked `True` it stays `True`; every subsequent retry re-enters the `all(...)` branch and never re-raises. So the request can no longer fail fast with `NoRouteFound` — it ends in `Timeout` (or in the `add_peer` exception if the connection itself fails). The new `run_test5`/`t5_1` correctly documents this (now expects `Timeout`). It's a deliberate trade-off, but callers that branched on `NoRouteFound` should be aware. Also note `add_peer` is retried on every round; harmless (`_add_peer` short-circuits if already connected) but worth knowing.
62+
63+
- **Default `True` privacy trade-off.** Defaulting to opening direct connections (revealing IP to the destination/introduction point) is debatable for a privacy-sensitive wallet. It's strictly better than the old unconditional behavior since there's now a toggle, and `add_peer` is proxy-aware (skips DNS when a proxy is set, connects over Tor), so the long-desc's "unless a proxy like Tor is used" caveat holds. Just flagging the default for a deliberate call.
64+
65+
- **`str(e.peer_address)` round-trip is correct.** `LNPeerAddr.__str__` → `pubkey_hex@host:port`, and `add_peer` parses exactly that via `extract_nodeid` + `split_host_port`. Fine.
66+
67+
- **Test gap.** `run_test5` covers single-dest direct-connect (on/off) well; `run_test6` covers multi-dest round-robin but only when *all* paths have routes (both are direct peers) — i.e. it exercises round-robin on reply-timeout, never `NoRouteFound`. The multi-path-all-fail → fallback scenario (the headline) is untested, which is why the dead-code path slipped through green tests.
68+
69+
- **Nit:** the truncated `# NOTE: above, when passing the caught exception … leads to GeneratorExit() in` comment in `process_send_queue` is now dangling but it's pre-existing, not from this commit.
70+
71+
## Recommendation
72+
73+
Land-worthy as an incremental reliability win for single-destination sends, *if* you either:
74+
75+
- **(a)** fix the multi-path control flow so intermediate `NoRouteFound`s resubmit instead of failing, and only fall back / fail once all paths are exhausted (plus a multi-path test); **or**
76+
- **(b)** drop the `route_not_found_for`/`all(...)` machinery and the "all blinded paths" wording until multi-path is genuinely handled — right now the code promises more than it does.
77+
78+
## Open follow-ups
79+
80+
- [ ] Decide between fix (a) and trim (b).
81+
- [ ] If (a): define the final-failure semantics when all paths fail and fallback is off/unavailable (`NoRouteFound` fast-fail vs. `Timeout`).
82+
- [ ] Add a multi-path-all-fail test exercising the fallback.
83+
- [ ] Confirm the desired default for `ONION_MESSAGE_OPEN_DIRECT_CONNECTIONS` (currently `True`).

‎review-f992c4999-blinded-payments.md‎

Lines changed: 142 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,142 @@
1+
# Code Review — `f992c4999` "lnonion: support payment path blinding"
2+
3+
- **Commit:** `f992c499997efa4c7abce75cfff619c27672075d`
4+
- **Author:** Sander van Grieken `<sander@outrightsolutions.nl>` (Co-Authored-By: f321x)
5+
- **Date:** 2025-11-11
6+
- **Branch:** `bolt12_path_blinding` (commit is deep in branch history; **key findings verified to still exist at branch HEAD**)
7+
- **Spec reference:** `bolts-lightning-specification/04-onion-routing.md`
8+
9+
## Scope & caveat
10+
11+
The commit prepares Electrum's Lightning code to **handle incoming blinded (BOLT-12 / route-blinding) payments**. Nothing here is currently exploitable for fund loss because:
12+
13+
- Incoming blinded **receive** is deliberately stubbed: `_check_accepted_final_htlc` raises `"we cannot receive blinded payments yet."`
14+
- Blinded **forwarding** runs only when `EXPERIMENTAL_LN_FORWARD_PAYMENTS` is enabled and Electrum is an intermediate hop in someone else's blinded path.
15+
16+
So treat severities as *"correctness within the new code path"* — i.e. the things to fix before enabling blinded receive/forward.
17+
18+
**Overall:** Well-structured commit; the core route-blinding cryptography is correct (intro-point vs non-intro key handling, the two shared-secret derivations, and the `E_{i+1}` chain all verified against BOLT-04). All issues are in peripheral logic — fee math, spec MUST-checks, caching, and error codes.
19+
20+
---
21+
22+
## 🔴 High
23+
24+
### [ ] H1 — Wrong forwarding-fee formula
25+
**File:** `electrum/lnworker.py`, `_maybe_forward_htlc` (blinded branch)
26+
27+
```python
28+
next_amount_msat_htlc = htlc.amount_msat
29+
next_amount_msat_htlc -= int(next_amount_msat_htlc * payment_relay.get('fee_proportional_millionths') / 1_000_000)
30+
next_amount_msat_htlc -= payment_relay.get('fee_base_msat')
31+
```
32+
33+
Computes `amt − floor(amt·prop/1e6) − base`, which is **not** the spec formula. BOLT-04 (`04-onion-routing.md:340`) mandates the inverse-rounded form:
34+
35+
```
36+
amt_to_forward = ((amount_msat − fee_base_msat)·1e6 + 1e6 + fee_proportional_millionths − 1) / (1e6 + fee_proportional_millionths)
37+
```
38+
39+
**Concrete failure** (amount=1_000_000, base=1000, prop=1000): Electrum forwards **998_000**, spec says **998_002** — it forwards *too little*, and the shortfall **compounds per hop**. The sender sizes the route with the spec formula, so the final recipient receives less than `amt_to_forward` and rejects the whole payment (`INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS`). Blinded payments routed *through* an Electrum hop cannot complete.
40+
41+
Two secondary problems on the same lines:
42+
- **Float division** (`/ 1_000_000`) loses integer precision once `amt·prop` exceeds 2⁵³ (~a 1 BTC HTLC with prop≈1e6), making the result non-deterministic vs other nodes.
43+
- **No underflow / insufficient-fee guard**: if `amount_msat < fee_base_msat`, `next_amount` goes negative (caught later as a misleading `TEMPORARY_CHANNEL_FAILURE` rather than a payload error).
44+
45+
**Proposed fix** — integer math matching the spec, plus a guard:
46+
```python
47+
prop = payment_relay['fee_proportional_millionths']
48+
base = payment_relay['fee_base_msat']
49+
next_amount_msat_htlc = ((htlc.amount_msat - base) * 1_000_000 + 1_000_000 + prop - 1) // (1_000_000 + prop)
50+
next_cltv_abs = htlc.cltv_abs - payment_relay['cltv_expiry_delta']
51+
if next_amount_msat_htlc <= 0 or htlc.amount_msat - next_amount_msat_htlc < base:
52+
raise OnionRoutingFailure(code=OnionFailureCode.INVALID_ONION_BLINDING, data=onion_hash)
53+
```
54+
55+
---
56+
57+
## 🟠 Medium
58+
59+
### [ ] M1 — Onion cache key omits `path_key`
60+
**File:** `electrum/lnpeer.py:3300`, `_process_incoming_onion_packet`
61+
62+
```python
63+
cache_key = sha256(onion_hash + payment_hash + bytes([is_trampoline])) # path_key not included
64+
```
65+
66+
`path_key` materially changes processing (re-blinds the private key, drives `blinded_path_recipient_data` / `next_path_key`), and for non-intro hops it arrives on the **wire** (`update_add_htlc_tlvs`), independently of the onion. Two HTLCs with the same onion+payment_hash but different wire `path_key` collide; the first cached `ProcessedOnionPacket` is returned for the second, so forwarding can run with the wrong scid/fees/next-path-key. Latent today (receive stubbed) but the forwarding path already consumes these cached fields.
67+
68+
**Proposed fix:**
69+
```python
70+
cache_key = sha256(onion_hash + payment_hash + bytes([is_trampoline]) + (path_key or b''))
71+
```
72+
73+
### [ ] M2 — `next_path_key_override` (encrypted_data TLV type 8) ignored
74+
**File:** `electrum/lnonion.py`, `process_onion_packet`
75+
76+
```python
77+
if path_key:
78+
next_path_key = next_blinding_from_shared_secret(path_key, recipient_data_shared_secret)
79+
```
80+
81+
BOLT-04:651 says the forwarder **MUST** use `next_path_key_override` as the next path key when present. The onion-*message* path (`onion_message.py`) already honors it; the payment path doesn't, so any blinded *payment* route using an override silently breaks at the next hop. (Real-world payment likelihood is low — the spec discourages overrides for payments — but it's a hard MUST.)
82+
83+
**Proposed fix:**
84+
```python
85+
if path_key:
86+
override = (blinded_path_recipient_data or {}).get('next_path_key_override', {}).get('path_key')
87+
next_path_key = override or next_blinding_from_shared_secret(path_key, recipient_data_shared_secret)
88+
```
89+
90+
### [ ] M3 — Missing forwarder MUST-checks (`payment_constraints`, `allowed_features`)
91+
**File:** `electrum/lnworker.py`, `_maybe_forward_htlc` (blinded branch)
92+
93+
The relay reads only `payment_relay`; BOLT-04:325-335 also requires checking `payment_constraints` (`max_cltv_expiry`, `htlc_minimum_msat`, TLV type 12) and `allowed_features` (type 14). Without them, a relayed HTLC that violates the path's own constraints is forwarded anyway. Add the checks before computing `next_amount_msat_htlc`.
94+
95+
### [ ] M4 — `next_node_id` next-hop not supported
96+
**File:** `electrum/lnonion.py`, `ProcessedOnionPacket.next_chan_scid`
97+
98+
Only `short_channel_id` is read from recipient_data; a blinded hop specifying `next_node_id` (TLV type 4) instead yields `next_chan_scid is None` → `INVALID_ONION_PAYLOAD`, failing a well-formed route. The mutual-exclusion MUST (error if both `short_channel_id` and `next_node_id` present) is also unenforced. Resolve `next_node_id` → channel when scid is absent; reject if both present.
99+
100+
---
101+
102+
## 🟡 Low / nits
103+
104+
### [ ] L1 — Wrong failure code for blinded-path errors
105+
Blinded failures (missing `current_path_key` → `get_ecdh(None)`, decrypt failures, etc.) surface as generic codes instead of the mandated `invalid_onion_blinding` (`0x8000|0x4000|24`). The uniform error exists precisely so a failure doesn't leak the node's position in the path. Map blinded-path onion failures to `INVALID_ONION_BLINDING`.
106+
107+
### [ ] L2 — Unused imports in `lnpeer.py`
108+
`get_ecdh` (line 24) and `decrypt_onionmsg_data_tlv` (line 38) are imported but never referenced (the decryption moved into `process_onion_packet`). Drop them.
109+
110+
### [ ] L3 — Redundant double-decrypt in `onion_message.py`
111+
`process_onion_packet` now populates `blinded_path_recipient_data`, but `process_onion_message_packet` re-derives the shared secret and decrypts again (and the `payload['encrypted_recipient_data'][...]` access is unguarded → `KeyError` on a malformed message). Reuse the already-decrypted field.
112+
113+
### [ ] L4 — Cosmetic
114+
- `def _get_from_payload` has a double space (`electrum/lnonion.py`).
115+
- `UpdateAddHtlc._validate` doesn't check `path_key` (33-byte point), though the wire decoder enforces length downstream.
116+
117+
---
118+
119+
## ✅ Verified correct (ruled out — no action needed)
120+
121+
- Intro-point uses the **real** node key; non-intro hops use the **blinded** key. Both ECDHs correct.
122+
- `next_path_key` uses `recipient_data_shared_secret` (the right ssᵢ), not the onion ss. `E_{i+1}=SHA256(E_i‖ss_i)·E_i` correct.
123+
- `onion_message.py` refactor is behaviorally equivalent to the old explicit `blinding_privkey` call.
124+
- `_check_accepted_final_htlc`: no unbound-`payment_secret_from_onion` path; `allowed_payload_keys` matches spec line 343.
125+
- TLV nestings correct: `path_id`→`data`, `short_channel_id`→`short_channel_id`, `total_amount_msat`→`total_msat`.
126+
- `payment_relay.get(...)` cannot return `None` (TLV reader populates all fields or raises) — no `None * int` crash.
127+
- **All four outer-onion** `_process_incoming_onion_packet` call sites pass `path_key`; trampoline-onion sites correctly don't.
128+
- `ProcessedOnionPacket`'s new required field `blinded_path_recipient_data` has a single construction site — no other constructor breaks.
129+
- `UpdateAddHtlc.path_key` round-trips through `astuple`/`from_tuple`; old 5-tuples stay compatible via the default.
130+
- `@staticmethod`→instance change on `_check_accepted_final_htlc` is safe (single `self.` call site, `lnpeer.py` ~2258).
131+
- `update_add_htlc_tlvs` send-kwarg / `payload["update_add_htlc_tlvs"]` read-key are both correct (TLV streams keyed by type-name).
132+
- Newly attaching the TLV stream to `update_add_htlc` decodes/encodes the no-TLV case fine; the stricter rejection of unknown *even* TLVs is spec-correct ("it's ok to be odd").
133+
134+
---
135+
136+
## Suggested order of work
137+
138+
1. **H1** (fee formula + guard) — required before blinded forwarding can interop.
139+
2. **M1** (cache key) — cheap, high-value hardening.
140+
3. **L2** (unused imports) — trivial cleanup.
141+
4. **M2 / M3 / M4 / L1** — spec-compliance for forwarding; do together when wiring up real forwarding.
142+
5. **L3 / L4** — cleanup.

0 commit comments

Comments
 (0)