Payjoin Fixes (Requires review)

[i5hi]

Some work from payjoin-upgrade was not included in develop. Starting this PR to review it and merge in useful work.

[ethicnology]

@claude can you review this whole PR ?

[claude]

Claude finished @ethicnology's task in 2m 19s —— View job

---

Review of PR #2499 — Payjoin Fixes

I reviewed the full diff vs origin/develop (~22k additions, 153 files) area by area: payjoin core engine, receive/send flows, settings + v13→v14 migration, announcements, transactions/labels/wallet. Verdict up front: this is careful, well-tested work that is close to mergeable — the session-resolution engine, migration, and status derivation are unusually solid — but there are 3 verified issues I'd fix before merge and a handful of worthwhile follow-ups.

CI is green, the branch is up to date with develop, FEATURES.md was updated (two graph inaccuracies noted below), and app_en.arb carries all new keys.

---

🔴 Should fix before merge

1. Two payjoin stream handlers lack the unhandled-error hardening this PR added to the third — lib/core/payjoin/data/repository/payjoin_repository_impl.dart:642-650 and :751-864
The constructor registers three bare .listen() callbacks (lines 119–121). _processPayjoinRequest was explicitly hardened (its comment at ~492 describes a DB throw escaping the listener and "stranding the session"), but its siblings weren't: in _processPayjoinProposal, the fetchSender (642) and update(payjoinModel) (650) awaits sit before the try at 657 (and the catch path's fetchSender/update at 724/735 are unprotected), while _processExpiredPayjoin has no try/catch at all (fetch at 751–753, update(expiredModel) at 864). A DB throw there becomes an unhandled zone error; since the sender poll cancels its timer before emitting, the proposal PSBT is never persisted and the session strands until app restart — exactly the failure mode the request-handler fix was written for. Fix this →

2. BIP-329 pubkey labels are validated as txid:vout and now hard-rejected before write — lib/features/labels/domain/label_entity.dart:29-33
LabelType.publicKey shares the txid:vout split-validation with input. Per BIP-329 a pubkey record's ref is a public key (and bip329_codec.dart:178-183 maps it straight through), so a 66-char compressed key fails _validateTxid and a 64-char x-only key throws RangeError at parts[1]. This bug is pre-existing, but the PR's validate-before-write makes it a hard rejection, ImportLabelsUsecase aborts the whole import on the first pubkey record, _mapRowsTolerantly silently drops legacy pubkey rows on read — and the new test (label_entity_test.dart:47, "accepts a well-formed publicKey reference (txid:vout)") cements the wrong shape. Fix this →

3. Toggle-off vs in-flight session creation race in the receive bloc — lib/features/receive/presentation/bloc/receive_bloc.dart:273-300, 795-827
_onPayjoinSettingChanged got a post-await re-check (if (state.payjoinGloballyEnabled != event.enabled) return; at ~919, verified) plus restartable(), but the two other session-creation sites didn't. In _onBitcoinStarted, after payjoinEnabled is read, _receiveWithPayjoinUsecase.execute(...) does a directory round trip; if the user toggles payjoin off during that window, _onPayjoinSettingChanged(false) sees state.payjoin == null and has nothing to clear, then _onBitcoinStarted resumes and unconditionally arms _watchPayjoin and emits — so the QR advertises a pj= endpoint with a live watcher while the chip shows disabled. Same window in _onNewAddressGenerated. Apply the same re-check before _watchPayjoin/emit. Fix this →

---

🟡 Worth addressing (minor)

Payjoin core (payjoin_repository_impl.dart)

• Fallback watcher keyed by BIP21 URI can cross-contaminate a retried send (:306-316, :1278): a sender's payjoin id is the URI and _watchForTransaction early-returns on containsKey, so re-scanning the same QR after a failed attempt leaves the old originalTxId armed; if that old original lands, _onOriginalTransactionSeen re-fetches by id and marks the new session aborted with the wrong original. Funds-bounded (both pay the same recipient) but contaminates the state machine.
• Post-broadcast throw triggers a knowingly-conflicting original broadcast (:686-701, :1128-1148): in _broadcastPsbt, the fetchSender null-throw and update(completedModel) come after the successful network broadcast; a throw there lands in the proposal handler's catch, which broadcasts the original against the payjoin tx already relayed. The isCompleted-wins derivation only covers the persisted case.
• Resume sweep retries a permanently-unbroadcastable original forever (:901-948): no attempt bound or age cutoff; logs SEVERE on every app start.
• Receiver poll loses in-memory PDK progress on a late throw (pdk_payjoin_datasource.dart:719, 742-767): the retry assumes the directory re-serves an already-delivered payload; if not, the session strands until expiry.

Receive/send

• Disabling payjoin only cancels the bloc's subscription and nulls state (receive_bloc.dart:930-932) — the repository exposes no session-cancel API, so the persisted receiver keeps polling until expiry and a sender who scanned the earlier QR can still complete a payjoin after the user turned it off. Worth a follow-up issue if not this PR.
• A session expiring with no sender engagement (originalTxBytes == null) still counts as isPayjoinFlowOwningNavigation (receive_state.dart:254-257), so a user quietly displaying a QR gets yanked to a "payjoin expired" screen. With user-configurable payjoinExpireAfterSec this is realistically reachable.
• _onPayjoinToggled failure is log-only (receive_bloc.dart:874-887) — a failed settings write means the chip tap does nothing visible.
• Latent Countdown dispose crash newly reachable: late Timer? timer is never assigned when until is already past (lib/core/widgets/timers/countdown.dart:24-36), so dispose() throws LateInitializationError; both new call sites (send_screen.dart:1753, payjoin-in-progress screen) can plausibly mount with a past deadline.

Transactions

• initByPayjoinId/initByWalletTxId stream listeners lack the isClosed guard the other three new listeners got (transaction_details_cubit.dart:90, 325) — emit-after-close becomes an unhandled async error.
• Concurrent _loadDetailsByPayjoinId runs aren't serialized — an older stalled run can clobber a newer full-tx view (self-healing on next sync); and a resolved payjoin whose tx isn't local can trigger up to three full wallet syncs per event.
• _broadcastTxIdForPayjoin swallows all exceptions unlogged (:450-452).

Settings / migration

• The debounced saves on the payjoin settings screen are cancelled, not flushed, on dispose (payjoin_settings_screen.dart:63-70) — a value typed <500 ms before leaving the screen is shown but never persisted.
• The five addColumn duplicate-column guards in schema_13_to_14.dart:36-72 swallow silently, while the createTable guard logs — inconsistent observability of the same event class.

Localization / FEATURES.md

• All new payjoin, announcement, and status strings exist only in en (and partially fr/de); the other ~24 locales fall back to English. Fine if translations arrive via a later pipeline — flagging in case they don't.
• FEATURES.md graph: ANNOUNCEMENTS --> TX_HISTORY doesn't match the code (the signal comes from core/wallet, not features/transactions), and the new WALLETS --> ANNOUNCEMENTS edge is missing (wallet_home_screen.dart:3 imports the carousel).

---

🟢 Verified solid (checked hard, no issues)

• Funds safety: every double-broadcast window I could construct resolves to conflicting txs that each pay the intended recipient; the completed-wins status derivation covers the persisted race; the manual-broadcast double-spend guard is genuinely closed and tested.
• Resume sweep vs live watchers: no double-processing — disjoint predicates, containsKey-idempotent watchers, per-session in-flight sets, terminal-state re-checks under the Lock.
• v13→v14 migration: column defaults, backfill, fresh-install seed path, and table/steps/schema-json parity all line up, and schema_v13_to_v14_test.dart inserts real v13 rows and asserts backfilled values — unusually well done.
• Stream hygiene: all new subscriptions cancelled in close()/dispose(), isClosed guards on bloc/cubit callbacks (except the two noted), broadcast controllers closed.
• Privacy: sender payjoin ids (full BIP21 URIs) hashed for logs; success navigation switched to txid so the URI stays out of router locations.
• Settings bounds enforced in domain usecases, not just UI; no raw colors (new onSuccess token in both palettes); no hardcoded user-facing strings.
• Tests are substantive, not decorative: 2k-line repository test covering resume sweeps, watcher families, expiry semantics; send/receive bloc tests driving real terminal-state flows; adapter tests on an in-memory drift DB. Gaps: nothing exercises a stream-handler throw (issue 1), the toggle race (issue 3), or same-URI resend reuse.

---

Summary

The payjoin session-resolution engine is the strongest part of this PR — nearly every race has an explicit re-fetch guard, a documented rationale citing an observed incident, and a test. The three fix-before-merge items are: carry the stream-handler hardening to the two sibling handlers, correct the BIP-329 pubkey validation (it now hard-rejects valid imports), and extend the toggle-race guard to the other two session-creation sites. Everything else is polish-level or a reasonable follow-up issue.

Review method: full diff read across five areas by parallel review agents, with the top findings re-verified directly against the code before posting.

[i5hi]

@claude please add a follow up review

[claude]

Claude finished @i5hi's task in 4m 45s —— View job

---

Follow-up review — verifying the fixes since the last review

Verdict: all three fix-before-merge findings from the previous review (above in this thread) are properly fixed, each with a regression test. I re-read every fix commit in full context against the live code, checked for regressions and new edge cases the fixes might introduce, and found nothing blocking. CI for the fix commits was still running at review time (run 30092274219) — worth a glance before merge, but the changes are small and mechanical.

Todo

• Identify commits pushed since the previous review
• Verify fix for issue 1 (stream-handler hardening in payjoin_repository_impl.dart)
• Verify fix for issue 2 (BIP-329 publicKey label validation)
• Verify fix for issue 3 (toggle-off race in receive_bloc.dart)
• Review the announcement carousel dots-strip fix (d44aa1b42)
• Re-check outstanding minor items from the previous review
• Post follow-up review

---

✅ Issue 1 — stream-handler hardening (0b7ee5b0f) — fixed correctly

_processPayjoinProposal and _processExpiredPayjoin are now thin try/catch wrappers delegating to ..Inner methods (payjoin_repository_impl.dart:634-651, :760-774), matching the pattern already used in _processPayjoinRequest. This covers everything the previous review flagged: the pre-try fetchSender/update awaits, the proposal handler's own catch path (whose original-broadcast fallback can itself throw — that now lands in the outer catch instead of the zone), and the entirely-unprotected expiry handler. The new tests throw from fetchSender inside each handler and additionally verify the repository still processes a subsequent event after the proposal-handler throw. Wrapping at the outermost level rather than patching individual awaits is the right call — no future edit inside the handler can reopen the hole.

✅ Issue 2 — BIP-329 publicKey validation (fa24ffb9e) — fixed correctly

LabelType.publicKey now gets _validatePublicKey (64/66/130 hex chars, hex-decodability via the already-imported convert package) instead of the txid:vout split (label_entity.dart:29-34, :87-104). The bonus fix landed too: _validateOutPoint throws LabelValidationException on a missing : separator instead of the RangeError from indexing split() output. The wrong-shape test was replaced with four correct ones (compressed key accepted, x-only accepted, txid:vout rejected for publicKey, separator-less input rejected with the typed exception). Downstream this un-breaks ImportLabelsUsecase for files with pubkey records and stops _mapRowsTolerantly from dropping valid pubkey rows.

One optional tightening: the validator accepts any hex of a plausible key length — it doesn't check the prefix byte (02/03 for 66-char compressed, 04 for 130-char uncompressed). For a label reference this leniency is defensible (BIP-329 tolerance over strictness), so take it or leave it.

✅ Issue 3 — toggle-off vs in-flight session creation (7c1d1f729) — fixed correctly

Both remaining session-creation sites now re-check the live state after the directory round trip: receive_bloc.dart:286 (_onBitcoinStarted) and :813 (_onNewAddressGenerated) compare state.payjoinGloballyEnabled (which _onPayjoinSettingChanged updates synchronously at :907 before any await) against the value captured pre-await, and skip both _watchPayjoin and surfacing the session when they disagree. I verified the interleaving actually works in bloc semantics — the two handlers are registered on different event types, so the setting-changed handler genuinely runs during _onBitcoinStarted's await — and the new test drives exactly that schedule with a Completer, asserting no payjoin surfaces and verifyNever on the watcher.

Two small residuals, neither blocking:

• Toggle off→on during the flight creates a session leak, not a state bug: the re-check passes (true == true), _onPayjoinSettingChanged(true) also creates a session, and _watchPayjoin cancels the prior subscription (receive_bloc.dart:1049) so the bloc tracks exactly one — but the loser session stays persisted and polls until expiry. This is the same class as the already-flagged "disable doesn't cancel the persisted session" follow-up; a repository-level session-cancel API would resolve both.
• The regression test covers _onBitcoinStarted only; _onNewAddressGenerated got the identical guard but no test. Fine given the code is a mirror image.

✅ Carousel dots-strip fix (d44aa1b42) — good

Reserving the 22px dots strip only when showDots is true (and folding it into the scaled height budget instead of a magic 112) is a clean improvement — the single-announcement card no longer carries dead space. Nit only: the height budget scales the strip by textScale while the actual bottom padding stays a fixed 22, so at text scales below 1.0 the card content loses ~2px vs. its scaled base height. Almost certainly absorbed by layout slack; not worth a change unless an overflow shows up in testing.

🟡 Previously-flagged minor items — status unchanged

The four commits only address the three blockers plus the carousel; the minor list from the previous review remains open. None block merge; the ones I'd prioritize as follow-up issues:

1. Persisted receiver session outlives a payjoin disable (no session-cancel API) — now also the residual of the toggle race above, so it covers two findings at once. Fix this →
2. Fallback watcher keyed by BIP21 URI can cross-contaminate a retried send (payjoin_repository_impl.dart _watchForTransaction early-return on containsKey).
3. Session expiring with no sender engagement still yanks navigation to the expired screen (receive_state.dart isPayjoinFlowOwningNavigation).
4. Countdown's late Timer? dispose crash when mounted with a past deadline; the isClosed guards missing from initByPayjoinId/initByWalletTxId listeners; the debounced payjoin-settings saves cancelled (not flushed) on dispose; translations beyond en/fr/de; the two FEATURES.md graph edges.

Summary

The fixes are exactly what the previous review asked for — no shortcuts, correct placement (outermost wrapping for the handlers, live-state re-check for the race), and each with a test that drives the actual failure schedule rather than a proxy. From my side this PR is mergeable once CI on the fix commits completes; the remaining minors are follow-up-issue material.

Note: I could not run the Flutter test suite locally in this environment (SDK checks require additional tool permissions); verification was by code reading, and CI run 30092274219 will confirm the suite.

---