Skip to content

Conversation

@ben-kaufman
Copy link
Contributor

@ben-kaufman ben-kaufman commented Jan 16, 2026

  • Prevent node from stopping immediately when going to background and fix app getting stuck on app status

  • Fix scanning of address to send while node is initializing

  • Fix turning off payment notifications from settings is not saved

@claude

This comment has been minimized.

@claude

This comment has been minimized.

ovitrif
ovitrif previously approved these changes Jan 16, 2026
Copy link
Collaborator

@ovitrif ovitrif left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utAck

maybe 1 little nit:
could use init instead of initialize/ initialization in fn names 🙏🏻

@claude

This comment has been minimized.

ovitrif
ovitrif previously approved these changes Jan 16, 2026
@claude

This comment has been minimized.

@claude

This comment has been minimized.

@claude

This comment has been minimized.

@claude

This comment has been minimized.

ovitrif
ovitrif previously approved these changes Jan 16, 2026
Copy link
Collaborator

@ovitrif ovitrif left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re utAck

settings.enableNotifications = true
notificationManager.requestPermission()
} else {
if newStatus != .authorized && settings.enableNotifications {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this change related to the rest of this PR? I fear that this change will break something. Can we split this up so it is easier to test/review?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not directly related the PR is mostly multiple bug fixes, we could separate it if you prefer

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes please, move at least this change out. I'm fairly certain this is there for a reason, and the whole notification registration flow has a lot of edge cases that need testing

Comment on lines 55 to 59
if let wallet {
_ = await wallet.waitForNodeToRun()
try? await Task.sleep(nanoseconds: Self.nodeReadyDelayNanoseconds)
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to wait for the node to parse scanning data. I have a working branch that moves some logic around to handle this. Can we move this fix out of the PR as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted the 2 commits mentioned

@claude
Copy link

claude bot commented Jan 16, 2026

Code review

Found 1 issue:

Race Condition in isSettingUp Flag

File: Bitkit/Services/LightningService.swift
Lines: 35, 44-48

The newly introduced isSettingUp flag has a race condition. The flag is a plain Bool in a non-actor-isolated class, and it is read on line 44 and written on line 48 without any synchronization.

// Guard against concurrent setup calls
guard !isSettingUp && node == nil else {
Logger.debug("Node already setting up or already set up, skipping")
return
}
isSettingUp = true
defer { isSettingUp = false }

Two concurrent calls to setup() could both pass the guard check (both seeing isSettingUp == false) before either sets it to true, defeating the purpose of the guard.

While StateLocker.lock() at line 52 would prevent both setups from running simultaneously, this happens after the race window, meaning:

  1. The second call will unnecessarily wait 30 seconds before timing out
  2. The isSettingUp flag's purpose (quick early return) is completely defeated

Suggested fixes:

  1. Make LightningService an actor, OR
  2. Mark isSettingUp as @MainActor isolated and ensure all calls to setup() happen on the main actor, OR
  3. Use a proper synchronization primitive like NSLock or an atomic operation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants