swaps: re-bundle_payments on startup

[ecdsa]

No description provided.

[f321x]

I ran Claude over this, only had some minor complaints. Imo it seems ok to merge either way.

See output (click to expand)

Re-bundle is not gated on "bundle already completed" — low

submarine_swaps.py:275-278

lnpeer deletes a bundle the moment it completes (lnpeer.py:3195), before either member can
settle. The gate at :273 does not know that, so a restart can resurrect a bundle whose prepay
member's mpp set is already fulfilled and evicted — and is_payment_bundle_complete then returns
False forever (lnworker.py:2793-2798).

Reachable via a narrow interleaving. lnpeer.py:2921 iterates all mpp sets in one pass:

1. Main visited first: reaches COMPLETE, stops at lnpeer.py:3189-3191 because the prepay set
   is still WAITING. set_mpp_resolution persists that (lnworker.py:3001-3002).
2. Prepay visited later in the same pass: bundle now complete → delete_payment_bundle
   (:3195) → SETTLING → _fulfill_htlc_set strips its htlcs synchronously
   (lnpeer.py:2295-2314) → len(htlcs) == 0 at :2938 → entry deleted in that same pass.
3. End of pass: main persisted as COMPLETE, prepay entry gone, bundle gone. Main flips to
   SETTLING only on the next pass (:3208), ~0.1 s later.

_check_final_mpp_set_state short-circuits SETTLING but not COMPLETE
(lnpeer.py:3228-3268), so an unclean stop inside that window leaves a set that, after the startup
re-bundle, is MPP-timed-out at lnpeer.py:3095-3101 using the persisted first_htlc_timestamp.
The base commit recovers correctly here (no bundle → is_payment_bundle_complete short-circuits to
True), so this is a regression.

Impact is bounded: no funding tx has been broadcast in that state, so the main HTLCs return to the
client. Loss is the client's prepay (2 × mining fee) plus a dead swap.

Ordering + swallowed exception can only produce the unsafe state — low

submarine_swaps.py:274-280

register_hold_invoice runs before bundle_payments, and except Exception logs and continues —
leaving a live hold invoice with no bundle, precisely the state this PR exists to eliminate. The
creation path does it the safe way round (bundle :809, register :750). The except is dead code
today: _payment_bundles_pkey_to_canon is empty at lnworker.py:1093, three statements before
SwapManager is constructed at :1097, so the only reachable assert needs two db swaps sharing a
hash. Drop it, or make failure safe by skipping registration.

The get_preimage hardening is only on the server path — low

create_normal_swap:725 is a good addition — payment_hash there is attacker-controlled
(request['preimageHash'], :1544). But the shared helper add_normal_swap is also reached by the
client at :1034 with a payment_hash chosen by the server (data["preimageHash"], :1000),
and :1075 hits the same assert get_preimage(...) is None (lnworker.py:2910). A malicious server
can pick a hash the client holds a public preimage for — clients store preimages for every payment
they have sent (lnchannel.py:1489/1730/1748, lnsweep.py:515), with no RECEIVED payment_info
to trip save_payment_info's guard (lnworker.py:2936-2938). Impact is a remote-triggerable
AssertionError plus a half-created swap in the db, not theft. The check belongs beside the existing
payment_hash.hex() in self._swaps at :766, covering both callers.

No test coverage — low

No unit test constructs a SwapManager from a persisted db; the four regtest swapserver tests
(tests/regtest.py:104-114) never restart the server.

Nits

• Duplicate "payment_hash already in use" message at :724 and :726 makes server logs ambiguous.
• Successful re-bundles are not logged at all.
• _prepayments[...] = ... at :272 / :810 silently overwrites where add_reverse_swap:904-905
  raises.
• 595d844 carries an unrelated precondition despite its subject; 02a8ccb's "simplification" also
  alters the re-bundle guard.

Suggested patch

if not swap.is_reverse and not swap.is_redeemed and not self.lnworker.get_preimage(swap.payment_hash):
    if (swap.prepay_hash is not None
            and self.lnworker.get_payment_status(swap.prepay_hash, direction=lnutil.RECEIVED) != PR_PAID):
        # re-bundle payments, because lnworker does not persist bundles.
        # note: if the prepay is already PR_PAID, lnpeer completed and deleted the bundle before
        #       shutdown; re-creating it would make is_payment_bundle_complete() permanently False.
        # TODO: drop this once lnworker persists _payment_bundles_* (lnworker.py:1093-1094)
        self.lnworker.bundle_payments([payment_hash, swap.prepay_hash])
    self.lnworker.register_hold_invoice(payment_hash, self.hold_invoice_callback)

Keeps the is_reverse nesting, skips already-completed bundles (fixes 1 and 2), and orders
bundle-before-register so a failure cannot leave the unsafe state (fixes 3). PR_PAID and lnutil
are already imported (:49, :41), and PR_PAID is durable — lnpeer.py:2930 sets it immediately
before _fulfill_htlc_set, ending in save_db().

Two gates that look equivalent but are not:

• not swap.is_funded() — _payment_pending is a non-persisted class default (:219) and
  funding_txid is only written once lnwatcher observes the output, so it reads False right after a
  restart, exactly when you need True.
• prepay key in received_mpp_htlcs — skips re-bundling in the case this commit exists to fix
  (client paid the main hold invoice, restart, prepay HTLCs not yet arrived), reintroducing the
  original bug.

Suggested test

tests/test_submarine_swaps.py, built on tests/lnhelpers.py's _create_mock_lnwallet: create a
swap via add_normal_swap(..., prepay=True), assert the bundle exists, construct a fresh
SwapManager over the same db, assert the bundle is restored; then mark the prepay PR_PAID and
assert it is not restored.