fix: hold a redo spark exit rebuild over a waiting leaf

Bitkit/Services/SparkExit/SparkExitDriver.swift

@@ -55,6 +55,8 @@ struct SparkExitDriver {
     /// A leaf the operators' watchtower stopped is relabelled promptly; one still stopped after this many blocks
     /// never will be.
     static let stoppedLeafBlocks: UInt32 = 6
+    /// Blocks a Redo rebuild waits for a skipped leaf the diverged generation recovers, about an hour.
+    static let redoHoldBlocks: UInt32 = 6
     /// A build waiting for a leaf that may still move tries again after this many blocks, doubling up to a day.
     static let retryBlocks: UInt32 = 6
     static let retryMaxBlocks: UInt32 = 144
@@ -386,7 +388,12 @@ struct SparkExitDriver {
         guard !prepared.leaves.isEmpty else {
             // A leaf the chain could not be read for, or one the operators' watchtower just stopped, may still move:
             // the build waits for it, for about a week. Finished leaves are not listed.
-            if prepared.skipped.contains(where: { isProvisional($0, prepared: prepared, record: record, tip: tip) }) {
+            let provisional = prepared.skipped.filter { isProvisional($0, prepared: prepared, record: record, tip: tip) }
+            // A Redo hold keeps its own pace and bound: the waiting leaf's retry clock would stretch it.
+            if holdsRedo(&record, waiting: provisional.map(\.leafId), tip: tip) {
+                return
+            }
+            if !provisional.isEmpty {
                 guard let tip else { return }
                 let since = record.retrySince ?? tip
                 if tip.saturatingSub(since) < Self.releaseBlocks {
@@ -439,6 +446,9 @@ struct SparkExitDriver {
             // Only cooperative leaves the operators could not co-sign: kept as the exit's recovery, so the next check
             // puts them through the bounded failed-leaf retry (about a week), with the operators-waiting line.
             if !built.failed.isEmpty {
+                if holdsRedo(&record, waiting: waitingLeafIds, tip: tip) {
+                    return
+                }
                 record.adopt(built, at: now())
                 record.waitingLeafIds = waitingLeafIds.isEmpty ? nil : waitingLeafIds
                 if let valid {
@@ -467,6 +477,11 @@ struct SparkExitDriver {
             }
             return
         }
+        // A diverged generation is never sent again, so a Redo rebuild that leaves out a waiting leaf it recovers would
+        // leave that leaf unsent until the rest of the exit is done: it tries again at its usual pace for a few blocks.
+        if holdsRedo(&record, waiting: waitingLeafIds, tip: tip) {
+            return
+        }
         record.adopt(built, at: now())
         record.waitingLeafIds = waitingLeafIds.isEmpty ? nil : waitingLeafIds
         if let valid, keepValidGeneration(&record, over: valid) {
@@ -993,6 +1008,27 @@ struct SparkExitDriver {
         return true
     }
 
+    /// A Redo rebuild that leaves out a waiting leaf the diverged generation recovers is not adopted for
+    /// `redoHoldBlocks` from the first such build: it stays rebuilding, at the rebuild pace.
+    private func holdsRedo(_ record: inout SparkExitRecord, waiting: [String], tip: UInt32?) -> Bool {
+        guard record.isRedoRebuild == true,
+              record.recovery?.response.leaves.contains(where: { waiting.contains($0.leafId) }) == true
+        else { return false }
+        // Without a tip the hold goes on uncounted: the failed read that skipped the leaf often fails the tip too.
+        let since = record.redoHeldSince ?? tip
+        if let tip, let since, tip.saturatingSub(since) >= Self.redoHoldBlocks {
+            return false
+        }
+        Logger.info("Holding the Spark exit's Redo rebuild for waiting leaves \(waiting)", context: "SparkExitDriver")
+        record.redoHeldSince = since
+        record.lastBuildAt = now()
+        // Funding a top-up already covered runs the hold at the rebuild pace, with no fee ask on screen.
+        if record.phase != .running, record.fundingShortfallSats == nil {
+            record.phase = .running
+        }
+        return true
+    }
+
     /// An unverified rebuild that adopted nothing keeps the valid generation's retry clock: one it set while waiting
     /// for the unread leaf would let that leaf go after about a week, ending the exit before its signed steps are sent.
     private func keepRetryClock(_ record: inout SparkExitRecord, from valid: SparkExitRecord) {

Bitkit/Services/SparkExit/SparkExitRecord.swift

@@ -247,6 +247,10 @@ struct SparkExitRecord: Codable, Equatable {
     /// The pending rebuild answers a divergence the SDK reported (`Redo`). Only such a rebuild adds lost-leaf strikes,
     /// so rebuilds over a stalled chain or an unverified step never make a leaf lost.
     var isRedoRebuild: Bool?
+    /// The tip at which a Redo rebuild first left out a waiting leaf the diverged generation recovers. The rebuild is
+    /// not adopted for `SparkExitDriver.redoHoldBlocks` from then, so a leaf one failed read skipped is not left unsent
+    /// until the rest of the exit is done.
+    var redoHeldSince: UInt32?
     /// The fee funding asked for came from a rebuild over a step the SDK could not read, while the generation out there
     /// is valid: funding it resumes that rebuild with its guard, and the ask ends once every step reads again.
     var isUnverifiedAsk: Bool?
@@ -428,6 +432,7 @@ struct SparkExitRecord: Codable, Equatable {
             rebuiltFromChainConfirmedCount = SparkExitMath.confirmedCount(built)
         }
         isRedoRebuild = nil
+        redoHeldSince = nil
         isUnverifiedAsk = nil
         isFeeBumpPending = nil
         feeBumpBuiltAtHeight = nil

BitkitTests/SparkExitTests.swift

@@ -330,6 +330,7 @@ final class SparkExitRecordTests: XCTestCase {
         XCTAssertNil(record.broadcastLog[0].isInputSpent)
         XCTAssertNil(record.leftoverSweep?.isConfirmed)
         XCTAssertNil(record.fundingPaymentTxid)
+        XCTAssertNil(record.redoHeldSince)
         XCTAssertTrue(record.showsResult, "an accepted sweep not known to be confirmed keeps the result")
 
         record.broadcastLog[0].isInputSpent = true
@@ -338,6 +339,7 @@ final class SparkExitRecordTests: XCTestCase {
         record.spentStepStrikes = ["r1": 1]
         record.lostLeaves = [.init(leafId: "leaf-1", valueSats: 5000)]
         record.outsideSats = 14167
+        record.redoHeldSince = 1000
         XCTAssertEqual(try SparkExitRecordCoder.decode(SparkExitRecordCoder.encode(record)), record)
     }
 
@@ -1382,6 +1384,170 @@ final class SparkExitDriverTests: XCTestCase {
         )
     }
 
+    /// Since SDK 0.28.2 a lost fee race comes as `Redo`. The refund whose input was spent is struck once; once the
+    /// rebuild's refund is sent, the next Redo strikes nothing and no leaf is lost.
+    func testARedoAfterALostFeeRaceStrikesTheSpentRefundOnlyOnce() async throws {
+        var record = SparkExitFixtures.record(phase: .running)
+        record.funding[0] = .init(index: 0, address: "bcrt1qfunding", pubkey: "02ab", txid: "f0", vout: 1, valueSats: 20000)
+        let raced = SparkExitFixtures.recovery([
+            SparkExitFixtures.tx("n1", status: .confirmed(blockHeight: 10)),
+            SparkExitFixtures.refund("r1", leafId: "leaf-1", dependsOn: ["n1"]),
+        ])
+        record.adopt(raced, at: .distantPast)
+        record.broadcastLog = [.init(
+            generation: 1,
+            txid: "r1",
+            attempts: 1,
+            rejections: 0,
+            lastAttemptAt: clock,
+            lastOutcome: "inputs missing",
+            isAccepted: false,
+            isInputSpent: true
+        )]
+        // The refund keeps its txid; its rebuilt child spends the funding that is still there.
+        var rebuilt = raced
+        rebuilt.transactions[1].cpfpTxHex = "03r1b"
+        sdk.checks = [
+            CheckRecoverFundsResponse(recovery: raced, verdict: .redo(reason: .onChainStateDiverged)),
+            CheckRecoverFundsResponse(recovery: rebuilt, verdict: .redo(reason: .onChainStateDiverged)),
+        ]
+        sdk.builds = [rebuilt, rebuilt]
+
+        record = try await driver().tick(record)
+        XCTAssertEqual(record.generation, 2)
+        XCTAssertEqual(record.spentStepStrikes, ["r1": 1])
+        XCTAssertEqual(chain.broadcasts.last?.cpfpTxHex, "03r1b", "the rebuilt refund was not sent")
+        XCTAssertEqual(record.broadcastLog.first { $0.txid == "r1" }?.isAccepted, true)
+
+        clock = clock.addingTimeInterval(SparkExitDriver.rebuildInterval)
+        record = try await driver().tick(record)
+        XCTAssertEqual(record.generation, 3)
+        XCTAssertNil(record.spentStepStrikes, "a second strike in a row for a refund its rebuild sent")
+        XCTAssertNil(record.lostLeaves)
+    }
+
+    /// A Redo whose rebuild skips leaf-2, unread once, while the diverged generation recovers it: the rebuild is not
+    /// adopted, so leaf-2 is not left unsent until the rest of the exit is done. It is built again at the rebuild pace
+    /// and adopted with both leaves once leaf-2 reads.
+    func testARedoRebuildBesideAWaitingLeafHoldsUntilTheLeafReads() async throws {
+        var record = try await heldRedoRebuild()
+        XCTAssertEqual(record.generation, 1, "the Redo rebuild without leaf-2 was adopted")
+        XCTAssertEqual(record.redoHeldSince, 1000)
+        XCTAssertEqual(saved.last?.redoHeldSince, 1000, "a relaunch would start the hold again")
+        XCTAssertTrue(record.isRebuilding)
+        XCTAssertTrue(chain.broadcasts.isEmpty, "the diverged generation was sent")
+
+        record = try await driver().tick(record)
+        XCTAssertEqual(sdk.recoverRequests.count, 1, "built again before the rebuild interval")
+
+        clock = clock.addingTimeInterval(SparkExitDriver.rebuildInterval)
+        var both = SparkExitFixtures.prepared(leaves: ["leaf-1", "leaf-2"])
+        both.skipped = []
+        sdk.preparedResponse = both
+        sdk.builds = [SparkExitFixtures.recovery([SparkExitFixtures.tx("n1b"), SparkExitFixtures.tx("m1b")])]
+        record = try await driver().tick(record)
+
+        XCTAssertEqual(sdk.recoverRequests.last?.prepared.leaves.map(\.leafId), ["leaf-1", "leaf-2"])
+        XCTAssertEqual(record.generation, 2)
+        XCTAssertNil(record.waitingLeafIds)
+        XCTAssertNil(record.redoHeldSince)
+        XCTAssertFalse(record.isRebuilding)
+    }
+
+    /// A leaf that stays unread holds the Redo rebuild for `redoHoldBlocks` only: then it is adopted with the leaf
+    /// waiting, tried again on its own before the exit ends. A tick whose tip cannot be read holds, uncounted.
+    func testARedoRebuildBesideALeafThatStaysUnreadAdoptsAfterTheHold() async throws {
+        var record = try await heldRedoRebuild()
+        sdk.builds = Array(repeating: SparkExitFixtures.recovery([SparkExitFixtures.tx("n1b")]), count: 3)
+
+        chain.isTipUnreadable = true
+        clock = clock.addingTimeInterval(SparkExitDriver.rebuildInterval)
+        record = try await driver().tick(record)
+        XCTAssertEqual(sdk.recoverRequests.count, 2)
+        XCTAssertEqual(record.generation, 1, "adopted on a tick whose tip could not be read")
+        XCTAssertEqual(record.redoHeldSince, 1000)
+        chain.isTipUnreadable = false
+
+        chain.tip = 1000 + SparkExitDriver.redoHoldBlocks - 1
+        clock = clock.addingTimeInterval(SparkExitDriver.rebuildInterval)
+        record = try await driver().tick(record)
+        XCTAssertEqual(record.generation, 1, "adopted before the hold ended")
+        XCTAssertEqual(record.redoHeldSince, 1000)
+
+        chain.tip = 1000 + SparkExitDriver.redoHoldBlocks
+        clock = clock.addingTimeInterval(SparkExitDriver.rebuildInterval)
+        record = try await driver().tick(record)
+        XCTAssertEqual(record.generation, 2)
+        XCTAssertEqual(record.waitingLeafIds, ["leaf-2"])
+        XCTAssertNil(record.redoHeldSince)
+        XCTAssertFalse(record.isRebuilding)
+    }
+
+    /// A Redo hold whose top-up was funded runs at the rebuild pace: it leaves the funding phase, so no fee ask stays on
+    /// screen and the next tick does not build again before the rebuild interval.
+    func testARedoHoldAfterAFundedTopUpRunsAtTheRebuildPace() async throws {
+        var record = try await heldRedoRebuild()
+        record.phase = .needsFunding(requiredSats: 23000)
+        chain.utxosByAddress["bcrt1qfunding"] = [
+            SparkExitFixtures.utxo("f0", vout: 1, value: 20000),
+            SparkExitFixtures.utxo("topup", value: 3000),
+        ]
+        sdk.builds = Array(repeating: SparkExitFixtures.recovery([SparkExitFixtures.tx("n1b")]), count: 2)
+        clock = clock.addingTimeInterval(SparkExitDriver.rebuildInterval)
+
+        record = try await driver().tick(record)
+        XCTAssertEqual(record.funding.compactMap(\.txid), ["f0", "topup"])
+        XCTAssertEqual(record.generation, 1, "the Redo rebuild without leaf-2 was adopted")
+        XCTAssertEqual(record.phase, .running, "a funded hold still asks for fees")
+        XCTAssertEqual(record.redoHeldSince, 1000)
+        let requests = sdk.recoverRequests.count
+
+        record = try await driver().tick(record)
+        XCTAssertEqual(sdk.recoverRequests.count, requests, "built again before the rebuild interval")
+    }
+
+    /// An empty prepare during a Redo hold keeps the hold's own bound: the waiting leaf's retry clock is not started.
+    func testAnEmptyPrepareDuringARedoHoldLeavesTheRetryClockAlone() async throws {
+        var record = try await heldRedoRebuild()
+        var none = SparkExitFixtures.prepared(leaves: [])
+        none.skipped = [SkippedLeaf(leafId: "leaf-2", valueSats: 50000, reason: .unverified)]
+        sdk.preparedResponse = none
+
+        clock = clock.addingTimeInterval(SparkExitDriver.rebuildInterval)
+        record = try await driver().tick(record)
+        XCTAssertNil(record.retrySince, "the hold started the waiting leaf's retry clock")
+        XCTAssertEqual(record.redoHeldSince, 1000)
+        XCTAssertEqual(record.generation, 1)
+        XCTAssertTrue(record.isRebuilding)
+
+        chain.tip = 1000 + SparkExitDriver.redoHoldBlocks
+        clock = clock.addingTimeInterval(SparkExitDriver.rebuildInterval)
+        record = try await driver().tick(record)
+        XCTAssertEqual(record.retrySince, chain.tip, "the waiting leaf's retry clock starts once the hold ends")
+    }
+
+    /// A Redo over a generation that recovers both leaves, whose rebuild skips leaf-2 as unread, at tip 1000.
+    private func heldRedoRebuild() async throws -> SparkExitRecord {
+        var record = twoLeafRecord()
+        var diverged = SparkExitFixtures.recovery([
+            SparkExitFixtures.tx("n1", status: .confirmed(blockHeight: 900)),
+            SparkExitFixtures.refund("r1", leafId: "leaf-1", dependsOn: ["n1"]),
+            SparkExitFixtures.tx("m1", status: .confirmed(blockHeight: 900)),
+            SparkExitFixtures.refund("r2", leafId: "leaf-2", dependsOn: ["m1"]),
+        ])
+        diverged.leaves = [
+            RecoverFundsLeaf(leafId: "leaf-1", valueSats: 100_000, method: .unilateral),
+            RecoverFundsLeaf(leafId: "leaf-2", valueSats: 50000, method: .unilateral),
+        ]
+        record.adopt(diverged, at: .distantPast)
+        sdk.checks = [CheckRecoverFundsResponse(recovery: diverged, verdict: .redo(reason: .onChainStateDiverged))]
+        var skipsLeafTwo = SparkExitFixtures.prepared(leaves: ["leaf-1"])
+        skipsLeafTwo.skipped = [SkippedLeaf(leafId: "leaf-2", valueSats: 50000, reason: .unverified)]
+        sdk.preparedResponse = skipsLeafTwo
+        sdk.builds = [SparkExitFixtures.recovery([SparkExitFixtures.tx("n1b")])]
+        return try await driver().tick(record)
+    }
+
     /// Top-ups join the exit under its own funding index, and the build signs with that index's key.
     func testFundingAndSigningUseTheRecordsFundingIndex() async throws {
         var record = SparkExitFixtures.record()