[ovitrif]

Twin: synonymdev/bitkit-android#1892

Description

• Fixes the emergency exit leaving a waiting leaf unsent after a Redo rebuild.
• Since Breez SDK 0.28.2 (chore: bump breez sdk to 0.28.2 #1306), a CPFP chain that lost a race comes back from check_recover_funds as Redo, not Valid. The driver never sends a diverged generation again, and its Redo rebuild skipped the hold that keeps a leaf in the exit when a build leaves it waiting. So when one failed chain read made the rebuild's prepare skip a leaf as Unverified, that leaf's steps went unsent until the rest of the exit was done, which can be weeks. Before 0.28.2 the same race went through the lost-race fallback rebuild, which kept that hold.
• A Redo rebuild that leaves out a waiting leaf the diverged generation recovers is now not adopted. It tries again at the usual rebuild pace for 6 blocks, so a leaf one failed read skipped is built with the rest. After 6 blocks it is adopted as before, with the leaf waiting. A tick that can't read the tip keeps the hold without counting it. The hold runs at that pace even when it follows a funded top-up, with no fee ask on screen, and a prepare that skips every leaf during it doesn't start the waiting leaf's week-long retry clock.
• The lost-race fallback for a race the SDK still calls Valid stays as it is.

Design

N/A — no UI changes.

Preview

N/A

QA Notes

Automated Checks

• SparkExitTests:
  • testARedoRebuildBesideAWaitingLeafHoldsUntilTheLeafReads: a Redo rebuild whose prepare skips leaf-2 as Unverified adopts nothing and sends nothing, then adopts both leaves once leaf-2 reads.
  • testARedoRebuildBesideALeafThatStaysUnreadAdoptsAfterTheHold: the hold keeps its start height for 6 blocks, then the rebuild is adopted with leaf-2 waiting.
  • testARedoHoldAfterAFundedTopUpRunsAtTheRebuildPace: a hold whose top-up was adopted leaves the funding phase and doesn't build again before the rebuild interval.
  • testAnEmptyPrepareDuringARedoHoldLeavesTheRetryClockAlone: a prepare that skips every leaf during the hold sets no retry clock; it starts once the hold ends.
  • testARedoAfterALostFeeRaceStrikesTheSpentRefundOnlyOnce: a Redo over a refund logged as input-spent strikes the refund once; after the rebuild's refund is accepted, a second Redo does not strike it again. Asked for by @jvsena42 on chore: bump breez sdk to 0.28.2 #1306 (review).
  • testReadsARecordWithoutTheNewFieldsAndRoundTripsThem: an older record reads redoHeldSince as nil, and a set value survives a round trip.
• These and the other Spark exit suites (SparkExitReviewTests, SparkExitSweepTests, SparkExitFeeBumpTests, SparkExitRerunTests) pass locally on a simulator.

[chatgpt-codex-connector]

Codex Review Summary: ✅ Completed

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-11-12T15:48:23.115126Z	320f0b9	PR opened

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 320f0b95bc

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[greptile-apps]

Confidence Score: 5/5

[Medium impact] The PR appears safe to merge; no actionable new issues were found.

Summary

The PR holds incomplete Redo rebuilds for six blocks so a temporarily unread leaf can rejoin the exit.

• Redo rebuilds wait briefly when they skip a leaf the old recovery still covers.
• The Redo race test checks that rebuilt refunds are not counted as spent twice.

Tests were reviewed but not run.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Redo rebuild] --> B{Leaves skipped but present in old recovery?}
    B -->|No| C[Adopt rebuild]
    B -->|Yes| D{Six blocks elapsed?}
    D -->|No or tip unreadable| E[Keep old record without broadcasting]
    E --> F[Wait for rebuild interval]
    F --> A
    D -->|Yes| G[Continue with normal waiting-leaf rules]

Loading

_{Reviews (3) · Last reviewed commit: "fix: redo hold keeps the rebuild pace af..." · Reviewed by Greptile}

[coreyphillips]

Two independent reviews, nothing blocking a merge.

worth doing, does not block

• Redo hold after adopted top-up rebuilds every tick and shows a ₿0 fee ask (Bitkit/Services/SparkExit/SparkExitDriver.swift:1008-1021)
• A funded Redo hold retains the funding phase (Bitkit/Services/SparkExit/SparkExitDriver.swift:1018-1020)
• An empty prepare extends the bounded Redo hold (Bitkit/Services/SparkExit/SparkExitDriver.swift:477-478)

nits

• Lost-race Redo test does not check which funding the rebuild spends (BitkitTests/SparkExitTests.swift:1387-1426)

[ovitrif]

Milestone 3.2.0: this PR is the iOS twin of synonymdev/bitkit-android#1892 and fixes the Spark emergency exit, which ships in 3.2.0 with #1128. It closes no issue, so the milestone sits on this PR.

[talosmachina]

No findings. Same hold as synonymdev/bitkit-android#1892: a Redo rebuild that leaves out a waiting leaf the diverged generation recovers stays rebuilding for redoHoldBlocks instead of being adopted. Reviewed 320f0b9 against feat/breez-spark-lightning, full tier, static: iOS does not build here, so this is reasoned from the code and CI.

What I checked, and 4 candidates I ruled out

Read: SparkExitDriver.build in full (both holdsRedo sites, the failed-only and full-build adopt paths, unexitable and lost-leaf exits), tick's .running where record.isRebuilding branch, SparkExitRecord.adopt, the new tests in SparkExitTests.swift
Twin: logic matches the Android PR line for line (6 blocks, saturatingSub where Android guards height >= since)

Ruled out

• Stale redoHeldSince reaching a later Redo: adopt clears it, and rebuildStalled and the unverified/fee-bump rebuilds only start from a running check, which never runs while rebuilding.
• Decoding old records: redoHeldSince is an optional on the synthesized Codable record, so records written before this change decode with nil.
• Rebuilding every tick while held: lastBuildAt = now() keeps isRebuildDue pacing it.
• Hold without a tip never ends: by design and commented; it starts counting at the first tip it reads.

CI: SwiftFormat and Greptile green; Run Tests still running at review time.

Merge confidence: 4/5, Run Tests had not finished and the change was not built here.

[ovitrif]

e11efd3 addresses @coreyphillips's review:

• A Redo hold after an adopted top-up now runs at the rebuild pace, with no fee ask on screen.
• An empty prepare during the hold no longer starts the waiting leaf's retry clock.
• The lost-race test's doc comment and QA note now claim only what the test checks.

Two new tests cover the first two. The Spark exit suites and SwiftFormat lint pass locally.

Re-review is needed only because of this push: 320f0b9...e11efd3

[jvsena42]

No findings. holdsRedo uses the same predicate as Android synonymdev/bitkit-android#1892:

• saturatingSub keeps the hold through a reorg.
• The tip and the SDK share Env.sparkChainServiceUrl(.bitcoin) on mainnet, so a hold with no readable tip isn't reachable while the SDK still reads.
• payHigherFee re-checks phase == .running, !isRebuilding before applyFeeBump (:861), so a held record can't be fee-bumped.
• Strikes are recomputed from the stored ones on each held build and saved only on adopt.

Device gate: no device check needed: needs a lost CPFP race; the driver tests cover it. perf: about 6 extra prepare/recover rounds per Redo episode.

Reviewer model: Claude Fable 5.2 (claude-fable-5-2), Claude Opus 5.5 (claude-opus-5-5)
Reasoning effort: not exposed

[coreyphillips]

Rechecked after 1 new commit (e11efd3). Nothing blocking remains.

addressed

• Redo hold after adopted top-up rebuilds every tick and shows a ₿0 fee ask (Bitkit/Services/SparkExit/SparkExitDriver.swift:1008-1021). Fixed: holdsRedo now sets the phase to .running once fundingShortfallSats is nil (SparkExitDriver.swift:1025-1028). The next tick therefore takes the .running where record.isRebuilding branch at :182-185, which is gated by isRebuildDue on the lastBuildAt set at :1024. In .running, reminders gets past :319 and plans the -rebuild reminder at SparkExitFunding.swift:330-332, and the screen no longer shows the needs-funding copy. I ran testARedoHoldAfterAFundedTopUpRunsAtTheRebuildPace on an iPhone 17e simulator at e11efd3 and it passes. Its phase == .running assertion would fail without the change.
• Lost-race Redo test does not check which funding the rebuild spends (BitkitTests/SparkExitTests.swift:1387-1426). Fixed: The doc comment at SparkExitTests.swift:1387-1388 now claims only the single strike, the sent refund and no lost leaf, and :1418-1426 assert each of those. The QA note was trimmed the same way. The fixture comment at :1407 ("spends the funding that is still there") still describes funding, but it explains the setup rather than claiming coverage.
• A funded Redo hold retains the funding phase (Bitkit/Services/SparkExit/SparkExitDriver.swift:1018-1020). Fixed: This is the same change as point 1, at SparkExitDriver.swift:1025-1028. A Redo hold whose top-up was adopted leaves .needsFunding, so the needsFunding else branch at :174-175 no longer builds on every tick. isAwaitingOwnPayment no longer matters for the funding- reminder, because .running never plans one. When a shortfall is still open (isRedoFromHeldFunding), the phase stays .needsFunding and :130-131 paces the build with isRebuildDue. The new test passes locally.
• An empty prepare extends the bounded Redo hold (Bitkit/Services/SparkExit/SparkExitDriver.swift:477-478). Fixed: In the empty-prepare branch, holdsRedo now runs on the provisional skips before retrySince/retriedAt are set (SparkExitDriver.swift:391-395). During the hold the build is paced by lastBuildAt and is not gated by the guard at :378-379. The retry clock starts only once redoHeldSince is redoHoldBlocks old. testAnEmptyPrepareDuringARedoHoldLeavesTheRetryClockAlone checks both halves and passes at e11efd3. All 89 SparkExitDriverTests pass.

The base branch was changed.

[ovitrif]

#1128 merged into master as 96225a1 and its branch was deleted, so this PR now targets master. Its diff is unchanged: the same 2 commits and 3 files, with no conflicts.

@coreyphillips @jvsena42 your approvals were on this same head (e11efd3), but master counts them as stale after the base change. Could you approve again? Nothing changed since your reviews.

[ovitrif]

The retarget e2e run stopped at its merge-checkout check: the checked-out merge no longer matched the PR base after the retarget to master. No test ran, so I reran it.

[coreyphillips]

Rechecked after replies, with no new commit since e11efd3. Nothing blocking remains.

[ovitrif]

Restarted the retarget E2E with a fresh dispatch: the earlier run and its rerun both stopped at the checkout check, because master moved after the merge sha was taken. No test had run.

[ovitrif]

Closing and reopening this PR to make GitHub rebuild its merge commit. It still merged onto the old Spark base (bbf02ca) after the retarget to master, so the retarget E2E stopped at its checkout check every time. Approvals and code stay as they are.

[ovitrif]

Reopened this PR twice more to clear stale checks. The runs the first reopen started had checked out the old merge commit and failed their checkout check, and a rerun repeats that commit. So the first reopen rebuilt the merge commit on current master (21e87d6), and the second started fresh E2E and integration runs on it. Code and approvals are unchanged.

[ovitrif]

Merging: approved on this head, required checks green after the merge ref was rebuilt on current master, no open threads.