Skip to content

fix: spark exit redo hold and overlapping sends #1323

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ovitrif merged 1 commit into from
Nov 13, 2026
Merged

fix: spark exit redo hold and overlapping sends #1323

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions Bitkit/Managers/SparkExitManager.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1576,10 +1576,14 @@ final class SparkExitManager {
UserDefaults.standard.dictionary(forKey: remindersKey) as? [String: TimeInterval] ?? [:]
}

/// An unverified rebuild's fee ask the tick from `previous` withdrew: the valid generation needs nothing more.
/// A fee ask the tick from `previous` withdrew: an unverified rebuild's, as the valid generation needs nothing more,
/// or one of a Redo rebuild, asked before or during it, that runs with no new funding, on the Redo's tick or a later
/// one, as the held funding pays for it.
nonisolated static func isFundingAskWithdrawn(_ record: SparkExitRecord, previous: SparkExitRecord) -> Bool {
guard previous.isUnverifiedAsk == true, case .needsFunding = previous.phase else { return false }
return record.phase == .running
guard case .needsFunding = previous.phase, record.phase == .running else { return false }
let isRedoAsk = (previous.isRedoAsk == true || record.isRedoAsk == true || previous.isRedoRebuild == true)
&& record.funding == previous.funding
return previous.isUnverifiedAsk == true || isRedoAsk
Comment thread
ovitrif marked this conversation as resolved.
Resolved
}

/// Takes back the exit's delivered fee asks, which no longer hold.
Expand Down
82 changes: 72 additions & 10 deletions Bitkit/Services/LightningService.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1078,7 +1078,12 @@ class LightningService: NodeLifecycleService {
return paid
}
guard beginRailSend(paymentHash: paymentHash) else { throw Self.pendingDuplicate(paymentHash: paymentHash) }
defer { endRailSend(paymentHash: paymentHash) }
var isRailSendEnded = false
defer {
if !isRailSendEnded {
endRailSend(paymentHash: paymentHash)
}
}
// Spark takes no route parameters, so a caller's routing fee cap is checked against its estimate instead.
if let maxFeeMsat = params?.maxTotalRoutingFeeMsat {
let feeSats: UInt64
Expand All @@ -1096,6 +1101,7 @@ class LightningService: NodeLifecycleService {
throw ChannelFreeRailSendRefused(reason: t("wallet__max_possible_fee_rate"))
}
}
var isRefusedAtAdmission = false
do {
let sent = try await rail.send(
bolt11: bolt11,
Expand All @@ -1104,7 +1110,12 @@ class LightningService: NodeLifecycleService {
?? Self.paymentAmountSats(bolt11: bolt11, sats: sats).map(Self.defaultMaxFeeSats),
beforeSubmission: { [weak self] in
try beforeSubmission()
try self?.admitSend(paymentHash: paymentHash, on: .channelFree, since: mark)
do {
try self?.admitSend(paymentHash: paymentHash, on: .channelFree, since: mark)
} catch {
isRefusedAtAdmission = true
throw error
}
// Noted once admitted, so a refused attempt never takes the place of the one that went out.
self?.noteAttempt(on: .channelFree, bolt11: bolt11)
self?.recordRailAttempt(paymentHash: paymentHash, expiresAt: expiresAt)
Expand All @@ -1125,6 +1136,16 @@ class LightningService: NodeLifecycleService {
releaseRailAttempt(paymentHash: paymentHash, unlessInFlight: false)
}
throw refusal
} catch where isRefusedAtAdmission {
// Nothing of this send left. Refused for a Spark send recorded since the checks, its caller gets that
// send's outcome, as it may miss its completion.
guard let paymentHash, eventCallbackLock.withLock({
attemptsRecorded(since: mark.records, key: Self.railAttemptsKey).contains(paymentHash.lowercased())
}) else { throw error }
endRailSend(paymentHash: paymentHash)
isRailSendEnded = true
guard let paid = try await settledRecordedRailSend(paymentHash: paymentHash, since: mark) else { throw error }
return paid
} catch {
// A send that failed once submitted may still go out, so its outcome waits for a later sync.
if let paymentHash {
Expand Down Expand Up @@ -1221,8 +1242,8 @@ class LightningService: NodeLifecycleService {

/// When the wallet was restored, in seconds since 1970. Spark can report a sync while its payment history failed to
/// sync, so its list can lack a send it made before then, and a still-valid invoice created before it that Spark
/// hasn't listed as pending or paid is refused on either rail until it expires, unless Spark's operators confirm
/// no send of it.
/// hasn't listed as pending or paid is refused on either rail until it expires, unless Spark's operators show no
/// live or paid send of it.
static let sparkRestoredAtKey = "sparkRestoredAt"

var sparkRestoredAt: Int? {
Expand Down Expand Up @@ -1458,7 +1479,8 @@ class LightningService: NodeLifecycleService {
/// duplicate, as LDK does its own, and a settled one returns its hash as paid. A failed or unknown one lets the
/// payment go. While either rail can't list its payments, an invoice it was sent over is refused as pending too.
/// After a restore, a still-valid invoice created (`invoiceCreatedAt`) before it that Spark's list doesn't show
/// pending or paid is refused unless Spark's operators confirm no send of it (`refuseUnverifiedRestoredInvoice`).
/// pending or paid is refused unless Spark's operators show no live or paid send of it
/// (`refuseUnverifiedRestoredInvoice`).
private func settledEarlierAttempt(
paymentHash: String?,
on rails: [LightningPaymentRail],
Expand Down Expand Up @@ -1546,8 +1568,9 @@ class LightningService: NodeLifecycleService {
}

/// After a restore, refuses a still-valid invoice created before it, as Spark's list can lack a send it made then,
/// unless Spark's operators show no Lightning send of it and it can't have been paid by a direct Spark transfer,
/// which they can't see. Any send they show, sent, pending or failed, or no answer, refuses it until it expires.
/// unless Spark's operators show no live or paid Lightning send of it and it can't have been paid by a direct Spark
/// transfer, which they can't see. A send they show sent or pending, or no answer, refuses it until it expires. A
/// failed one counts as unsent, as they show it failed only once every attempt at the hash was returned.
private func refuseUnverifiedRestoredInvoice(
paymentHash: String,
bolt11: String,
Expand All @@ -1566,15 +1589,15 @@ class LightningService: NodeLifecycleService {
guard sparkRuns else { return }
do {
guard let rail else { throw ChannelFreeRailUnavailable() }
if let status = try await rail.lightningSendStatus(bolt11: bolt11) {
if let status = try await rail.lightningSendStatus(bolt11: bolt11), status != .failed {
Logger.warn("Refused paying '\(paymentHash)', a pre-restore invoice Spark shows as '\(status)'", context: "LightningService")
throw ChannelFreeRailSendRefused.restoredInvoiceUnverified()
}
if try await rail.mayPayBySparkTransfer(bolt11: bolt11, sats: sats) {
Logger.warn("Refused paying '\(paymentHash)', a pre-restore invoice Spark may have paid by transfer", context: "LightningService")
throw ChannelFreeRailSendRefused.restoredInvoiceUnverified()
}
Logger.info("Spark's operators show no send of pre-restore invoice '\(paymentHash)'", context: "LightningService")
Logger.info("Found no live or paid Spark operator send of pre-restore invoice '\(paymentHash)'", context: "LightningService")
} catch let refusal as ChannelFreeRailSendRefused {
throw refusal
} catch {
Expand Down Expand Up @@ -1673,7 +1696,8 @@ class LightningService: NodeLifecycleService {

/// The last check before a send leaves on `rail`, under the same lock that marks the other rail's sends in flight,
/// so of two sends of one invoice only the first admitted goes out. Refuses while the other rail has one in flight,
/// or recorded one since `mark`. An admitted node send stays in flight until `endLdkSend`; a Spark send already is.
/// or recorded one since `mark`, and a Spark send also once Spark recorded one since `mark`, as Spark doesn't refuse
/// a repeat. An admitted node send stays in flight until `endLdkSend`; a Spark send already is.
func admitSend(paymentHash: String?, on rail: LightningPaymentRail, since mark: AttemptMark) throws {
guard let hash = paymentHash?.lowercased() else { return }
try eventCallbackLock.withLock {
Expand All @@ -1683,12 +1707,50 @@ class LightningService: NodeLifecycleService {
Logger.warn("Refused paying '\(hash)', the other rail sent it meanwhile", context: "LightningService")
throw Self.pendingDuplicate(paymentHash: hash)
}
if rail == .channelFree, attemptsRecorded(since: mark.records, key: Self.railAttemptsKey).contains(hash) {
Logger.warn("Refused paying '\(hash)', Spark sent it meanwhile", context: "LightningService")
throw Self.pendingDuplicate(paymentHash: hash)
}
if rail == .ldk {
ldkSendsInFlight.insert(hash)
}
}
}

/// The outcome of the Spark send of `paymentHash` recorded since `mark` that refused this one at admission: its hash
/// once Spark lists it paid, the failure once it lists it failed with no send of the invoice live before or after
/// the listing is read and none recorded while it is, otherwise nil, as it may still pay.
private func settledRecordedRailSend(paymentHash: String, since mark: AttemptMark) async throws -> PaymentHash? {
let hash = paymentHash.lowercased()
// Read before the liveness check, so a send recorded right after that check still counts as recorded meanwhile.
let listingMark = attemptMark
let wasLive = isSendLive(paymentHash: hash, since: mark)
guard let payments = await readableRailPayments(),
let send = ChannelFreeRailPayment.latestOutboundAttempt(payments, paymentHash: hash)
else { return nil }
let isLive = wasLive || isSendLive(paymentHash: hash, since: mark) || eventCallbackLock.withLock {
attemptsRecorded(since: listingMark.records, key: Self.railAttemptsKey).contains(hash)
}
switch send.status {
case .succeeded:
Logger.info("Settled the refused Spark send '\(hash)' as paid", context: "LightningService")
return paymentHash
case .failed where !isLive:
Logger.info("Settled the refused Spark send '\(hash)' as failed", context: "LightningService")
throw ChannelFreeRailSendRefused(reason: t("wallet__payment_failed_description"))
default:
return nil
}
}

/// Whether a send of `hash` is in flight on either rail, or the node recorded one since `mark`.
private func isSendLive(paymentHash hash: String, since mark: AttemptMark) -> Bool {
eventCallbackLock.withLock {
railSendsInFlight.contains(hash) || ldkSendsInFlight.contains(hash)
|| attemptsRecorded(since: mark.records, key: Self.ldkAttemptsKey).contains(hash)
}
}

func beginLdkSend(paymentHash: String?) {
guard let paymentHash else { return }
_ = eventCallbackLock.withLock { ldkSendsInFlight.insert(paymentHash.lowercased()) }
Expand Down
23 changes: 17 additions & 6 deletions Bitkit/Services/SparkExit/SparkExitDriver.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,10 @@ struct SparkExitDriver {
@discardableResult
func tick(_ record: SparkExitRecord) async throws -> SparkExitRecord {
var record = record
// A Redo's fee ask ended on an earlier tick, so its withdrawal was already seen.
if record.isRedoAsk == true, record.phase.requiredFundingSats == nil {
record.isRedoAsk = nil
}
switch record.phase {
case .done, .failed:
guard record.needsDriving, !Task.isCancelled else { return record }
Expand Down Expand Up @@ -338,6 +342,7 @@ struct SparkExitDriver {
record.isRebuilding = true
record.isRedoRebuild = true
record.isUnverifiedAsk = nil
record.isRedoAsk = true
}
}

Expand Down Expand Up @@ -446,7 +451,7 @@ 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) {
if holdsRedo(&record, waiting: waitingLeafIds, tip: tip, isBuilt: true) {
return
}
record.adopt(built, at: now())
Expand Down Expand Up @@ -479,7 +484,7 @@ struct SparkExitDriver {
}
// 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) {
if holdsRedo(&record, waiting: waitingLeafIds, tip: tip, isBuilt: true) {
return
}
record.adopt(built, at: now())
Expand Down Expand Up @@ -754,6 +759,10 @@ struct SparkExitDriver {
utxos = try? await chain.utxos(address: address)
}
record.phase = .needsFunding(requiredSats: SparkExitMath.neededFunding(requiredSats: requiredSats, record: record, utxos: utxos))
// A Redo rebuild's ask ends once the funding it holds pays for a later build.
if record.isRedoRebuild == true {
record.isRedoAsk = true
}
}

/// Leaves a build skipped while they may still move (`waitingLeafIds`), prepared again on their own once the rest of
Expand Down Expand Up @@ -1009,8 +1018,9 @@ struct SparkExitDriver {
}

/// 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 {
/// `redoHoldBlocks` from the first such build: it stays rebuilding, at the rebuild pace. `isBuilt` when the funding
/// the exit holds paid for the build.
private func holdsRedo(_ record: inout SparkExitRecord, waiting: [String], tip: UInt32?, isBuilt: Bool = false) -> Bool {
guard record.isRedoRebuild == true,
record.recovery?.response.leaves.contains(where: { waiting.contains($0.leafId) }) == true
else { return false }
Expand All @@ -1022,8 +1032,9 @@ struct SparkExitDriver {
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 {
// A build the held funding paid for, or funding a top-up already covered, runs the hold at the rebuild pace, with
// no fee ask on screen; a later build that is short asks again.
if record.phase != .running, isBuilt || record.fundingShortfallSats == nil {
record.phase = .running
}
return true
Expand Down
3 changes: 3 additions & 0 deletions Bitkit/Services/SparkExit/SparkExitRecord.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,9 @@ struct SparkExitRecord: Codable, Equatable {
/// 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?
/// The fee funding asked for came while a Redo rebuild was pending: that rebuild may run on the funding the exit
/// holds, and the ask then ends. Kept until a tick starts past that ask.
var isRedoAsk: Bool?
/// The tip of the last rebuild over a step the SDK could not read, so it rebuilds at most once a block.
var unverifiedRebuiltAtHeight: UInt32?
/// A higher fee rate the steps out there are not built at yet: a fee bump whose build kept the valid generation
Expand Down
Loading
Loading