Skip to content

fix: avoid spurious node refetch after unlocking - #4423

Open
myxmaster wants to merge 3 commits into
ZeusLN:masterfrom
myxmaster:fix/redundant-refresh-after-unlock
Open

fix: avoid spurious node refetch after unlocking#4423
myxmaster wants to merge 3 commits into
ZeusLN:masterfrom
myxmaster:fix/redundant-refresh-after-unlock

Conversation

@myxmaster

Copy link
Copy Markdown
Collaborator

Description

I noticed this minor issue:
Opening and closing the menu once after app start triggered a full node refetch, with a visible loading indicator, although nothing had been changed. A second open/close was clean. Only reproducible with a lock configured (PIN, password or biometrics).

Tracing it turned up two further problems in the same mechanism, fixed in the follow-up commits.

resetAuthenticationAttempts() was fire-and-forget. updateSettings() persists to storage before it sets triggerSettingsRefresh, so on a successful login the flag landed after proceed() had already returned to the Wallet screen and its handleFocus() had cleared it. The flag then survived until the next focus event (first menu close) and forced the refetch there.
-> Now awaited at all four call sites.

updateSettings() armed triggerSettingsRefresh unconditionally, so writes re-persisting an identical value cost a full refetch too. Two happen on every normal app start: supportedBiometryType and authenticationAttempts: 0. Now guarded by an isEqual() check against the settings read at the start of the call.

Mostly theoretical, because probably not reproducible in practice, but fixed anyway since it is a simple fix:
handleFocus() also cleared the flags in the branch where the _navigating guard skipped the call. A settings change arriving while a refresh was in flight was therefore consumed without ever being applied. Clearing now happens only in the branch that actually starts getSettingsAndNavigate().

Testing

Verified on Android with PIN and with biometrics: no refetch on the first menu open/close. Settings changes that do require a reconnect still trigger one.
Please test on iOS.

This pull request is categorized as a:

  • New feature
  • Bug fix
  • Code refactor
  • Configuration change
  • Locales update
  • Quality assurance
  • Other

Checklist

  • I’ve run yarn run tsc and made sure my code compiles correctly
  • I’ve run yarn run lint and made sure my code didn’t contain any problematic patterns
  • I’ve run yarn run prettier and made sure my code is formatted correctly
  • I’ve run yarn run test and made sure all of the tests pass

Testing

If you modified or added a utility file, did you add new unit tests?

  • No, I’m a fool
  • Yes
  • N/A

I have tested this PR on the following platforms (please specify OS version and phone model/VM):

  • Android
  • iOS

I have tested this PR with the following types of nodes (please specify node version and API version where appropriate):

On-device

  • LDK Node
  • Embedded LND

Remote

  • LND (REST)
  • LND (Lightning Node Connect)
  • Core Lightning (CLNRest)
  • Nostr Wallet Connect
  • LndHub

Locales

  • I’ve added new locale text that requires translations
  • I’m aware that new translations should be made on the ZEUS Transfix page and not directly to this repo

Third Party Dependencies and Packages

  • Contributors will need to run yarn after this PR is merged in
  • 3rd party dependencies have been modified:
    • verify that package.json and yarn.lock have been properly updated
    • verify that dependencies are installed for both iOS and Android platforms

Other:

  • Changes were made that require an update to the README
  • Changes were made that require an update to onboarding

@kaloudis kaloudis added this to the v13.2.1 milestone Aug 14, 2026
@kaloudis

Copy link
Copy Markdown
Contributor

Concept ACK fa23cdd

Traced all three mechanisms against master and the diagnosis checks out: updateSettings() does persist before arming triggerSettingsRefresh, so the fire-and-forget reset's flag can land after Wallet's handleFocus() already cleared it; the legit post-unlock refetch (proceed() arming the flag before pop()) then absorbs the now-awaited write. All four new await sites are in async functions. Nice writeup, the three-commit split made this easy to verify.

Needs rebase. The branch is conflicting: master's applySettingsUpdate gained a persisted early-return guard (data-wipe write latch) that rewrites the exact lines this PR touches. Mechanical fix: the isEqual guard belongs after if (!persisted) return this.settings;. Semantics compose cleanly since blocked writes already skip arming the flag.

One real gap in the isEqual guard: devices without biometrics. getSupportedBiometryType() returns undefined when no sensor is available, so Wallet's componentDidMount writes { supportedBiometryType: undefined } on every start. JSON.stringify drops the key when persisting, but the merged in-memory object keeps it, and lodash isEqual({ a: 1 }, { a: 1, b: undefined }) is false. So on no-biometry devices this every-start write still arms the flag, which defeats half of commit 2's stated purpose. Today it happens to be absorbed by the initialLoad refetch, but combined with commit 3's leave-armed behavior it can resurface as a deferred refetch. Suggest stripping undefined-valued keys from resolvedSetting before the merge/compare (or comparing against the JSON round-trip of newSettings), which also matches what actually gets persisted.

Non-blocking observations:

  • Unlocks preceded by a failed attempt still trigger a refetch: the increment and the reset back to 0 are both real changes. Not a regression, but it suggests bookkeeping keys (authenticationAttempts, supportedBiometryType) arguably should never arm the refresh at all; an exclusion list would be the more complete fix. Same one-time arm the first time authenticationAttempts: 0 is written to a blob that never had the key.
  • posWasEnabled has the same disease: applySettingsUpdate still sets it unconditionally for any pos write with posEnabled !== Disabled, even a no-op re-persist. Follow-up candidate.
  • Commit 3's trade-off: a settings change racing an in-flight refresh is now deferred to the next focus event, which can be the next menu close, i.e. the same visible symptom in a rarer path. Optional improvement: re-check the flags in the .finally() and re-trigger immediately.
  • On the tests checkbox: stores/SettingsStore.test.ts already has the exact harness needed (seedSettings/StorageMock/persistedSettings, with existing triggerSettingsRefresh assertions in the blocked-write test). Two cheap cases would lock in commit 2: a no-op write leaves the flag false, a real change sets it. A third could pin down the undefined-key behavior.

Can run it through iOS once the rebase lands.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants